[Qemu-devel] [PATCH 0/2] qapi: Allow blockdev-add for NBD

[Qemu-devel] [PATCH 0/2] qapi: Allow blockdev-add for NBD

[Max Reitz]

While there are more formats still missing blockdev-add support, I
personally felt like NBD really is one we do need, and also it was
pretty simple to do.

Therefore, I started with NBD. Maybe I'll follow up with other formats.


Max Reitz (2):
  block/nbd: Reject port parameter without host
  qapi: Allow blockdev-add for NBD

 block/nbd.c          |  4 ++++
 qapi/block-core.json | 28 ++++++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

-- 
2.7.0

[Qemu-devel] [PATCH 1/2] block/nbd: Reject port parameter without host

[Max Reitz]

This is better than the generic block layer finding out later that the
port parameter has not been used.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/nbd.c b/block/nbd.c
index 1a90bc7..063c403 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -201,6 +201,10 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
         }
         return NULL;
     }
+    if (qdict_haskey(options, "port") && !qdict_haskey(options, "host")) {
+        error_setg(errp, "port may not be used without host.");
+        return NULL;
+    }
 
     saddr = g_new0(SocketAddress, 1);
 
-- 
2.7.0

Re: [Qemu-devel] [PATCH 1/2] block/nbd: Reject port parameter without host

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]

On 02/03/2016 09:33 AM, Max Reitz wrote:
> This is better than the generic block layer finding out later that the
> port parameter has not been used.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/nbd.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 1a90bc7..063c403 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -201,6 +201,10 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
>          }
>          return NULL;
>      }
> +    if (qdict_haskey(options, "port") && !qdict_haskey(options, "host")) {
> +        error_setg(errp, "port may not be used without host.");

No trailing '.'

With that fixed,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/2] block/nbd: Reject port parameter without host

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 977 bytes --]

On 03.02.2016 17:38, Eric Blake wrote:
> On 02/03/2016 09:33 AM, Max Reitz wrote:
>> This is better than the generic block layer finding out later that the
>> port parameter has not been used.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/nbd.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 1a90bc7..063c403 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -201,6 +201,10 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
>>          }
>>          return NULL;
>>      }
>> +    if (qdict_haskey(options, "port") && !qdict_haskey(options, "host")) {
>> +        error_setg(errp, "port may not be used without host.");
> 
> No trailing '.'

I plead guilty. I was tempted by the error_setg() calls above this one,
I'll add a patch to fix them in v2.

> With that fixed,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks,

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH 2/2] qapi: Allow blockdev-add for NBD

[Max Reitz]

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(-)

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',
-- 
2.7.0

Re: [Qemu-devel] [PATCH 2/2] qapi: Allow blockdev-add for NBD

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2694 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/2] qapi: Allow blockdev-add for NBD

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4359 bytes --]

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...


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 2/2] qapi: Allow blockdev-add for NBD

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 2/2] qapi: Allow blockdev-add for NBD

[Daniel P. Berrange]

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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 2/2] qapi: Allow blockdev-add for NBD

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 2/2] qapi: Allow blockdev-add for NBD

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 2/2] qapi: Allow blockdev-add for NBD

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 2/2] qapi: Allow blockdev-add for NBD

[Daniel P. Berrange]

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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|