| Message ID | 1454517196-4560-3-git-send-email-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/03/2016 09:33 AM, Max Reitz wrote: > We have to introduce a new object (BlockdevOptionsNbd) for several > reasons: > - Neither of InetSocketAddress nor UnixSocketAddress alone is > sufficient, because both are supported > - We cannot use SocketAddress because NBD does not support an fd, > and because it is not a flat union which BlockdevOptionsNbd is Can we do it anyways, and just error out/document that fd is unsupported? > - We cannot use a flat union of InetSocketAddress and > UnixSocketAddress because we would need some kind of discriminator > which we do not have; we could inline the UnixSocketAddress as a > string and then make it an 'alternate' type instead of a union, but > this will not work either, because: > - InetSocketAddress itself is not suitable for NBD because the port is > not optional (which it is for NBD) and because it offers more options > (like choosing between ipv4 and ipv6) which NBD does not support. That, and qapi doesn't (yet) support the use of a flat union as the branch of yet another flat union. I'd like to reach the point where we can have a flat union with an implicit discriminator (if the discriminator was not present, the require a default branch), but don't think it should hold up this patch. I also think that future qapi improvements may make it possible to retrofit this struct to make the mutual exclusion between host/file more obvious during introspection, rather than just by documentation. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > qapi/block-core.json | 28 ++++++++++++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > ## > +# @BlockdevOptionsNbd > +# > +# Driver specific block device options for NBD. Either of @host or @path must be > +# specified, but not both. > +# > +# @host: #optional Connects to the given host using TCP. > +# > +# @port: #optional Specifies the TCP port to connect to; may be used only in > +# conjunction with @host. Defaults to 10809. > +# > +# @path: #optional Connects to the given Unix socket path. > +# > +# @export: #optional Name of the NBD export to open. Maybe mention that the default is no export name. > +# > +# Since: 2.6 > +## > +{ 'struct': 'BlockdevOptionsNbd', > + 'data': { '*host': 'str', > + '*port': 'str', > + '*path': 'str', > + '*export': 'str' } } I'm not entirely convinced this is the final representation we want, but I can't immediately propose anything nicer.
On 03.02.2016 17:48, Eric Blake wrote: > On 02/03/2016 09:33 AM, Max Reitz wrote: >> We have to introduce a new object (BlockdevOptionsNbd) for several >> reasons: >> - Neither of InetSocketAddress nor UnixSocketAddress alone is >> sufficient, because both are supported >> - We cannot use SocketAddress because NBD does not support an fd, >> and because it is not a flat union which BlockdevOptionsNbd is > > Can we do it anyways, and just error out/document that fd is unsupported? Would be possible, if InetSocketAddress's port was optional and if it was a flat union. (Note that the port not being optional is not a real issue; it just means the user cannot omit it when using blockdev-add, but that's not so bad.) >> - We cannot use a flat union of InetSocketAddress and >> UnixSocketAddress because we would need some kind of discriminator >> which we do not have; we could inline the UnixSocketAddress as a >> string and then make it an 'alternate' type instead of a union, but >> this will not work either, because: >> - InetSocketAddress itself is not suitable for NBD because the port is >> not optional (which it is for NBD) and because it offers more options >> (like choosing between ipv4 and ipv6) which NBD does not support. > > That, and qapi doesn't (yet) support the use of a flat union as the > branch of yet another flat union. > > I'd like to reach the point where we can have a flat union with an > implicit discriminator (if the discriminator was not present, the > require a default branch), but don't think it should hold up this patch. > I also think that future qapi improvements may make it possible to > retrofit this struct to make the mutual exclusion between host/file more > obvious during introspection, rather than just by documentation. The problem here is that we really just want to merge and flatten {Inet,Unix}SocketAddress into a single union. The discriminator basically is of which object all the non-optional fields are present. >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> qapi/block-core.json | 28 ++++++++++++++++++++++++++-- >> 1 file changed, 26 insertions(+), 2 deletions(-) >> > >> ## >> +# @BlockdevOptionsNbd >> +# >> +# Driver specific block device options for NBD. Either of @host or @path must be >> +# specified, but not both. >> +# >> +# @host: #optional Connects to the given host using TCP. >> +# >> +# @port: #optional Specifies the TCP port to connect to; may be used only in >> +# conjunction with @host. Defaults to 10809. >> +# >> +# @path: #optional Connects to the given Unix socket path. >> +# >> +# @export: #optional Name of the NBD export to open. > > Maybe mention that the default is no export name. Can do (and will do, because you asked for it), but I thought that "Not specifying an export name means no export name" to be self-evident. :-) >> +# >> +# Since: 2.6 >> +## >> +{ 'struct': 'BlockdevOptionsNbd', >> + 'data': { '*host': 'str', >> + '*port': 'str', >> + '*path': 'str', >> + '*export': 'str' } } > > I'm not entirely convinced this is the final representation we want, but > I can't immediately propose anything nicer. Ideally, this would just be a SocketAddress. Unfortunately, this is not possible because SocketAddress is not flat but this has to be. The representation I guess I'd want under the circumstances would be a flat union of InetSocketAddress and UnixSocketAddress with a semantic discriminator as described above (check which non-optional fields are present). However, in order for this to work, the code generator would need to support flat unions with such a semantic discriminator[1], and also the @port parameter in InetSocketAddress should be optional (which might require non-trivial changes to existing code that expects an InetSocketAddress). However, as written above, the port not being optional would not be too bad. But in any case, the on-wire interface is stable and defined by block/nbd.c already, so I believe we can enrich the definition later on. Maybe we should define @port to be non-optional if @host is specified, though. Max [1] And I don't believe I feel quite comfortable with extending the code generator...
On Wed, Feb 03, 2016 at 05:33:16PM +0100, Max Reitz wrote: > We have to introduce a new object (BlockdevOptionsNbd) for several > reasons: > - Neither of InetSocketAddress nor UnixSocketAddress alone is > sufficient, because both are supported > - We cannot use SocketAddress because NBD does not support an fd, > and because it is not a flat union which BlockdevOptionsNbd is With my patch series that converts NBD to use QIOChannel, all the entry points for client & server *do* take a SocketAddress struct to provide address info. So internally the code does in fact allow use of an FD, if there were a way to specify it a the QAPI level... eg see https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04159.html > - We cannot use a flat union of InetSocketAddress and > UnixSocketAddress because we would need some kind of discriminator > which we do not have; we could inline the UnixSocketAddress as a > string and then make it an 'alternate' type instead of a union, but > this will not work either, because: > - InetSocketAddress itself is not suitable for NBD because the port is > not optional (which it is for NBD) and because it offers more options > (like choosing between ipv4 and ipv6) which NBD does not support. The *should* support ipv4 and ipv6 options for NBD. We should also make the port optional in the SocketAddress struct - I tried to do that previously but my patch was flawed, but we should revisit this. So IMHO all the things you list above are reasons *for* using SocketAddress and not re-inventing it poorly with explicit host + port fields. Regards, Daniel
On 03.02.2016 18:06, Daniel P. Berrange wrote: > On Wed, Feb 03, 2016 at 05:33:16PM +0100, Max Reitz wrote: >> We have to introduce a new object (BlockdevOptionsNbd) for several >> reasons: >> - Neither of InetSocketAddress nor UnixSocketAddress alone is >> sufficient, because both are supported >> - We cannot use SocketAddress because NBD does not support an fd, >> and because it is not a flat union which BlockdevOptionsNbd is > > With my patch series that converts NBD to use QIOChannel, all the > entry points for client & server *do* take a SocketAddress struct > to provide address info. So internally the code does in fact allow > use of an FD, if there were a way to specify it a the QAPI level... > > eg see > > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04159.html > >> - We cannot use a flat union of InetSocketAddress and >> UnixSocketAddress because we would need some kind of discriminator >> which we do not have; we could inline the UnixSocketAddress as a >> string and then make it an 'alternate' type instead of a union, but >> this will not work either, because: >> - InetSocketAddress itself is not suitable for NBD because the port is >> not optional (which it is for NBD) and because it offers more options >> (like choosing between ipv4 and ipv6) which NBD does not support. > > The *should* support ipv4 and ipv6 options for NBD. We should also make > the port optional in the SocketAddress struct - I tried to do that previously > but my patch was flawed, but we should revisit this. > > So IMHO all the things you list above are reasons *for* using SocketAddress > and not re-inventing it poorly with explicit host + port fields. That's good news, thanks! However, the issue remains that the NBD block driver expects flattened options which is syntactically incompatible to SocketAddress. Maybe the best way to address this would be to just make block/nbd.c directly accept a SocketAddress and keep the old flattened @host, @port, and @path options only as a legacy mapping to inet.host, inet.port, and unix.path. Anyway, I'll wait for your series to get merged, then. Max
On 03/02/2016 17:48, Eric Blake wrote: > On 02/03/2016 09:33 AM, Max Reitz wrote: >> We have to introduce a new object (BlockdevOptionsNbd) for >> several reasons: - Neither of InetSocketAddress nor >> UnixSocketAddress alone is sufficient, because both are >> supported - We cannot use SocketAddress because NBD does not >> support an fd, and because it is not a flat union which >> BlockdevOptionsNbd is > > Can we do it anyways, and just error out/document that fd is > unsupported? Especially because there's no reason _not_ to support fd. Sure, it's really fringe, but if qemu-socket APIs make it just work... Paolo
On 03/02/2016 18:16, Max Reitz wrote: > However, the issue remains that the NBD block driver expects > flattened options which is syntactically incompatible to > SocketAddress. Maybe the best way to address this would be to just > make block/nbd.c directly accept a SocketAddress and keep the old > flattened @host, @port, and @path options only as a legacy mapping > to inet.host, inet.port, and unix.path. Do we need to keep them at all? The URL-based file is already good enough as a shortcut for human and command-line use. Is anyone actually using host/port/path? Paolo
Am 03.02.2016 um 18:06 hat Daniel P. Berrange geschrieben: > On Wed, Feb 03, 2016 at 05:33:16PM +0100, Max Reitz wrote: > > We have to introduce a new object (BlockdevOptionsNbd) for several > > reasons: > > - Neither of InetSocketAddress nor UnixSocketAddress alone is > > sufficient, because both are supported > > - We cannot use SocketAddress because NBD does not support an fd, > > and because it is not a flat union which BlockdevOptionsNbd is > > With my patch series that converts NBD to use QIOChannel, all the > entry points for client & server *do* take a SocketAddress struct > to provide address info. So internally the code does in fact allow > use of an FD, if there were a way to specify it a the QAPI level... > > eg see > > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04159.html That's patch 1 of a series that has a few more dependencies. Can the patch be applied without the rest of the series (and without the dependencies) so that we don't have to wait for a very long time with Max's patches? > > - We cannot use a flat union of InetSocketAddress and > > UnixSocketAddress because we would need some kind of discriminator > > which we do not have; we could inline the UnixSocketAddress as a > > string and then make it an 'alternate' type instead of a union, but > > this will not work either, because: > > - InetSocketAddress itself is not suitable for NBD because the port is > > not optional (which it is for NBD) and because it offers more options > > (like choosing between ipv4 and ipv6) which NBD does not support. > > The *should* support ipv4 and ipv6 options for NBD. We should also make > the port optional in the SocketAddress struct - I tried to do that previously > but my patch was flawed, but we should revisit this. > > So IMHO all the things you list above are reasons *for* using SocketAddress > and not re-inventing it poorly with explicit host + port fields. Agreed. Anything in SocketAddress that isn't supported is either a bug or a missing feature. Kevin
On Thu, Feb 04, 2016 at 02:08:23PM +0100, Kevin Wolf wrote: > Am 03.02.2016 um 18:06 hat Daniel P. Berrange geschrieben: > > On Wed, Feb 03, 2016 at 05:33:16PM +0100, Max Reitz wrote: > > > We have to introduce a new object (BlockdevOptionsNbd) for several > > > reasons: > > > - Neither of InetSocketAddress nor UnixSocketAddress alone is > > > sufficient, because both are supported > > > - We cannot use SocketAddress because NBD does not support an fd, > > > and because it is not a flat union which BlockdevOptionsNbd is > > > > With my patch series that converts NBD to use QIOChannel, all the > > entry points for client & server *do* take a SocketAddress struct > > to provide address info. So internally the code does in fact allow > > use of an FD, if there were a way to specify it a the QAPI level... > > > > eg see > > > > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04159.html > > That's patch 1 of a series that has a few more dependencies. Can the > patch be applied without the rest of the series (and without the > dependencies) so that we don't have to wait for a very long time with > Max's patches? Paolo ackd the main nbd series, so we're just waiting for the CLI patch series it depends on https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00296.html In terms of merging the NBD series, the bare minimum is the qom patch and the --object arg support. I could rebase the NBD series to just include those two directly, since they're basically ready: https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00297.html https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00302.html Eric ACK'd the second one, and the fixes for the first one were trivial. Regards, Daniel
diff --git a/qapi/block-core.json b/qapi/block-core.json index 33012b8..e1b8543 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1567,13 +1567,14 @@ # Drivers that are supported in block device operations. # # @host_device, @host_cdrom: Since 2.1 +# @nbd: Since 2.6 # # Since: 2.0 ## { 'enum': 'BlockdevDriver', 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', 'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device', - 'http', 'https', 'null-aio', 'null-co', 'parallels', + 'http', 'https', 'nbd', 'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } @@ -2002,6 +2003,29 @@ '*read-pattern': 'QuorumReadPattern' } } ## +# @BlockdevOptionsNbd +# +# Driver specific block device options for NBD. Either of @host or @path must be +# specified, but not both. +# +# @host: #optional Connects to the given host using TCP. +# +# @port: #optional Specifies the TCP port to connect to; may be used only in +# conjunction with @host. Defaults to 10809. +# +# @path: #optional Connects to the given Unix socket path. +# +# @export: #optional Name of the NBD export to open. +# +# Since: 2.6 +## +{ 'struct': 'BlockdevOptionsNbd', + 'data': { '*host': 'str', + '*port': 'str', + '*path': 'str', + '*export': 'str' } } + +## # @BlockdevOptions # # Options for creating a block device. @@ -2027,7 +2051,7 @@ 'http': 'BlockdevOptionsFile', 'https': 'BlockdevOptionsFile', # TODO iscsi: Wait for structured options -# TODO nbd: Should take InetSocketAddress for 'host'? + 'nbd': 'BlockdevOptionsNbd', # TODO nfs: Wait for structured options 'null-aio': 'BlockdevOptionsNull', 'null-co': 'BlockdevOptionsNull',
We have to introduce a new object (BlockdevOptionsNbd) for several reasons: - Neither of InetSocketAddress nor UnixSocketAddress alone is sufficient, because both are supported - We cannot use SocketAddress because NBD does not support an fd, and because it is not a flat union which BlockdevOptionsNbd is - We cannot use a flat union of InetSocketAddress and UnixSocketAddress because we would need some kind of discriminator which we do not have; we could inline the UnixSocketAddress as a string and then make it an 'alternate' type instead of a union, but this will not work either, because: - InetSocketAddress itself is not suitable for NBD because the port is not optional (which it is for NBD) and because it offers more options (like choosing between ipv4 and ipv6) which NBD does not support. Signed-off-by: Max Reitz <mreitz@redhat.com> --- qapi/block-core.json | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)