[Qemu-devel] [RFC v2 for-2.9 00/10] Fixes and cleanups around SocketAddress

[Qemu-devel] [RFC v2 for-2.9 00/10] Fixes and cleanups around SocketAddress

[Markus Armbruster]

What makes this 2.9 material is the crash bug fixed in PATCH 2 and the
QAPI/QMP interface cleanups in PATCH 7+8.

This is RFC because:

1. To give you one more chance to ask for undocumented -drive
   driver=nbd usage compatibility [PATCH 08+09].

2. Another round of sheepdog tests is still in progress (with
   Kashyap's help).

Max, please have a close look at PATCH 11, I hope this is what you
meant when you asked for "parsing @server".

v2:
* PATCH 01+02+04+05 unchanged.
* PATCH 03 error message improved, blank line [Max]
* PATCH 06 commit message typo [Max]
* PATCH 07 new, factored out of old PATCH 7
* PATCH 08-10 updated, please re-review

Markus Armbruster (10):
  nbd sockets vnc: Mark problematic address family tests TODO
  char: Fix socket with "type": "vsock" address
  io vnc sockets: Clean up SocketAddressKind switches
  block: Document -drive problematic code and bugs
  gluster: Prepare for SocketAddressFlat extension
  qapi-schema: SocketAddressFlat variants 'vsock' and 'fd'
  sockets: New helper socket_address_crumple()
  nbd: Tidy up blockdev-add interface
  squash! nbd: Tidy up blockdev-add interface
  sheepdog: Fix blockdev-add

 block.c                | 41 ++++++++++++++++++++++--
 block/file-posix.c     |  6 ++++
 block/gluster.c        | 28 +++++++++++------
 block/nbd.c            | 62 ++++++++++++++++++++++---------------
 block/nfs.c            |  7 +++++
 block/rbd.c            |  6 ++++
 block/sheepdog.c       | 84 +++++++++++++++++++++++++++++++++-----------------
 block/ssh.c            |  8 +++++
 blockdev-nbd.c         |  1 +
 chardev/char-socket.c  |  9 ++++--
 include/qemu/sockets.h | 11 +++++++
 io/dns-resolver.c      |  7 +++--
 qapi-schema.json       | 19 +++++-------
 qapi/block-core.json   |  6 ++--
 ui/vnc.c               | 19 ++++++++----
 util/qemu-sockets.c    | 37 ++++++++++++++++++++--
 16 files changed, 258 insertions(+), 93 deletions(-)

-- 
2.7.4

[Qemu-devel] [RFC v2 for-2.9 01/10] nbd sockets vnc: Mark problematic address family tests TODO

[Markus Armbruster]

Certain features make sense only with certain address families.  For
instance, passing file descriptors requires AF_UNIX.  Testing
SocketAddress's saddr->type == SOCKET_ADDRESS_KIND_UNIX is obvious,
but problematic: it can't recognize AF_UNIX when type ==
SOCKET_ADDRESS_KIND_FD.

Mark such tests of saddr->type TODO.  We may want to check the address
family with getsockname() there.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/nbd.c           | 1 +
 blockdev-nbd.c        | 1 +
 chardev/char-socket.c | 5 ++---
 ui/vnc.c              | 1 +
 util/qemu-sockets.c   | 4 ++++
 5 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 1b832c2..36ea617 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -421,6 +421,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
             goto error;
         }
 
+        /* TODO SOCKET_ADDRESS_KIND_FD where fd has AF_INET or AF_INET6 */
         if (s->saddr->type != SOCKET_ADDRESS_KIND_INET) {
             error_setg(errp, "TLS only supported over IP sockets");
             goto error;
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 7ea836b..8a11807 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -124,6 +124,7 @@ void qmp_nbd_server_start(SocketAddress *addr,
             goto error;
         }
 
+        /* TODO SOCKET_ADDRESS_KIND_FD where fd has AF_INET or AF_INET6 */
         if (addr->type != SOCKET_ADDRESS_KIND_INET) {
             error_setg(errp, "TLS is only supported with IPv4/IPv6");
             goto error;
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index d7e92e1..6344b07 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -47,7 +47,6 @@ typedef struct {
     int max_size;
     int do_telnetopt;
     int do_nodelay;
-    int is_unix;
     int *read_msgfds;
     size_t read_msgfds_num;
     int *write_msgfds;
@@ -825,7 +824,6 @@ static void qmp_chardev_open_socket(Chardev *chr,
     int64_t reconnect   = sock->has_reconnect ? sock->reconnect : 0;
     QIOChannelSocket *sioc = NULL;
 
-    s->is_unix = addr->type == SOCKET_ADDRESS_KIND_UNIX;
     s->is_listen = is_listen;
     s->is_telnet = is_telnet;
     s->do_nodelay = do_nodelay;
@@ -865,7 +863,8 @@ static void qmp_chardev_open_socket(Chardev *chr,
     s->addr = QAPI_CLONE(SocketAddress, sock->addr);
 
     qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE);
-    if (s->is_unix) {
+    /* TODO SOCKET_ADDRESS_FD where fd has AF_UNIX */
+    if (addr->type == SOCKET_ADDRESS_KIND_UNIX) {
         qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
     }
 
diff --git a/ui/vnc.c b/ui/vnc.c
index 821acdd..fe0a46a 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3642,6 +3642,7 @@ static int vnc_display_connect(VncDisplay *vd,
         error_setg(errp, "Expected a single address in reverse mode");
         return -1;
     }
+    /* TODO SOCKET_ADDRESS_KIND_FD when fd has AF_UNIX */
     vd->is_unix = saddr[0]->type == SOCKET_ADDRESS_KIND_UNIX;
     sioc = qio_channel_socket_new();
     qio_channel_set_name(QIO_CHANNEL(sioc), "vnc-reverse");
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 40164bf..9b73681 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1154,6 +1154,10 @@ int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
 {
     int fd;
 
+    /*
+     * TODO SOCKET_ADDRESS_KIND_FD when fd is AF_INET or AF_INET6
+     * (although other address families can do SOCK_DGRAM, too)
+     */
     switch (remote->type) {
     case SOCKET_ADDRESS_KIND_INET:
         fd = inet_dgram_saddr(remote->u.inet.data,
-- 
2.7.4

[Qemu-devel] [RFC v2 for-2.9 02/10] char: Fix socket with "type": "vsock" address

[Markus Armbruster]

Watch this:

    $ qemu-system-x86_64 -nodefaults -S -display none -qmp stdio
    {"QMP": {"version": {"qemu": {"micro": 91, "minor": 8, "major": 2}, "package": " (v2.8.0-1195-gf84141e-dirty)"}, "capabilities": []}}
    { "execute": "qmp_capabilities" }
    {"return": {}}
    { "execute": "chardev-add", "arguments": { "id": "chr0", "backend": { "type": "socket", "data": { "addr": { "type": "vsock", "data": { "cid": "CID", "port": "P" }}}}}}
    Aborted (core dumped)

Crashes because SocketAddress_to_str() is blissfully unaware of
SOCKET_ADDRESS_KIND_VSOCK.  Fix that.  Pick the output format to match
socket_parse(), just like the existing formats.

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 chardev/char-socket.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 6344b07..36ab0d6 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -357,6 +357,10 @@ static char *SocketAddress_to_str(const char *prefix, SocketAddress *addr,
         return g_strdup_printf("%sfd:%s%s", prefix, addr->u.fd.data->str,
                                is_listen ? ",server" : "");
         break;
+    case SOCKET_ADDRESS_KIND_VSOCK:
+        return g_strdup_printf("%svsock:%s:%s", prefix,
+                               addr->u.vsock.data->cid,
+                               addr->u.vsock.data->port);
     default:
         abort();
     }
-- 
2.7.4

[Qemu-devel] [RFC v2 for-2.9 03/10] io vnc sockets: Clean up SocketAddressKind switches

[Markus Armbruster]

We have quite a few switches over SocketAddressKind.  Some have case
labels for all enumeration values, others rely on a default label.
Some abort when the value isn't a valid SocketAddressKind, others
report an error then.

Unify as follows.  Always provide case labels for all enumeration
values, to clarify intent.  Abort when the value isn't a valid
SocketAddressKind, because the program state is messed up then.

Improve a few error messages while there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 io/dns-resolver.c   |  7 +++++--
 ui/vnc.c            | 18 ++++++++++++------
 util/qemu-sockets.c |  4 +---
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/io/dns-resolver.c b/io/dns-resolver.c
index 0ac6b23..a407075 100644
--- a/io/dns-resolver.c
+++ b/io/dns-resolver.c
@@ -164,9 +164,12 @@ int qio_dns_resolver_lookup_sync(QIODNSResolver *resolver,
                                                 addrs,
                                                 errp);
 
+    case SOCKET_ADDRESS_KIND_FD:
+        error_setg(errp, "Unsupported socket address type 'fd'");
+        return -1;
+
     default:
-        error_setg(errp, "Unknown socket address kind");
-        return -1;
+        abort();
     }
 }
 
diff --git a/ui/vnc.c b/ui/vnc.c
index fe0a46a..0be77ba 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -129,10 +129,13 @@ static void vnc_init_basic_info(SocketAddress *addr,
         info->family = NETWORK_ADDRESS_FAMILY_UNIX;
         break;
 
-    default:
-        error_setg(errp, "Unsupported socket kind %d",
-                   addr->type);
+    case SOCKET_ADDRESS_KIND_VSOCK:
+    case SOCKET_ADDRESS_KIND_FD:
+        error_setg(errp, "Unsupported socket address type %s",
+                   SocketAddressKind_lookup[addr->type]);
         break;
+    default:
+        abort();
     }
 
     return;
@@ -411,10 +414,13 @@ VncInfo *qmp_query_vnc(Error **errp)
             info->family = NETWORK_ADDRESS_FAMILY_UNIX;
             break;
 
-        default:
-            error_setg(errp, "Unsupported socket kind %d",
-                       addr->type);
+        case SOCKET_ADDRESS_KIND_VSOCK:
+        case SOCKET_ADDRESS_KIND_FD:
+            error_setg(errp, "Unsupported socket address type %s",
+                       SocketAddressKind_lookup[addr->type]);
             goto out_error;
+        default:
+            abort();
         }
 
         info->has_host = true;
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 9b73681..4ae37bd 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1337,9 +1337,7 @@ char *socket_address_to_string(struct SocketAddress *addr, Error **errp)
         break;
 
     default:
-        error_setg(errp, "socket family %d unsupported",
-                   addr->type);
-        return NULL;
+        abort();
     }
     return buf;
 }
-- 
2.7.4

Re: [Qemu-devel] [RFC v2 for-2.9 03/10] io vnc sockets: Clean up SocketAddressKind switches

[Daniel P. Berrange]

On Thu, Mar 30, 2017 at 03:15:00PM +0200, Markus Armbruster wrote:
> We have quite a few switches over SocketAddressKind.  Some have case
> labels for all enumeration values, others rely on a default label.
> Some abort when the value isn't a valid SocketAddressKind, others
> report an error then.
> 
> Unify as follows.  Always provide case labels for all enumeration
> values, to clarify intent.  Abort when the value isn't a valid
> SocketAddressKind, because the program state is messed up then.
> 
> Improve a few error messages while there.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  io/dns-resolver.c   |  7 +++++--
>  ui/vnc.c            | 18 ++++++++++++------
>  util/qemu-sockets.c |  4 +---
>  3 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/io/dns-resolver.c b/io/dns-resolver.c
> index 0ac6b23..a407075 100644
> --- a/io/dns-resolver.c
> +++ b/io/dns-resolver.c
> @@ -164,9 +164,12 @@ int qio_dns_resolver_lookup_sync(QIODNSResolver *resolver,
>                                                  addrs,
>                                                  errp);
>  
> +    case SOCKET_ADDRESS_KIND_FD:
> +        error_setg(errp, "Unsupported socket address type 'fd'");
> +        return -1;
> +
>      default:
> -        error_setg(errp, "Unknown socket address kind");
> -        return -1;
> +        abort();
>      }
>  }

Just repeating my v1 comments - this needs to be treating KIND_FD as a no-op,
rather than raising an error.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [RFC v2 for-2.9 03/10] io vnc sockets: Clean up SocketAddressKind switches

[Max Reitz]

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

On 03.04.2017 13:48, Daniel P. Berrange wrote:
> On Thu, Mar 30, 2017 at 03:15:00PM +0200, Markus Armbruster wrote:
>> We have quite a few switches over SocketAddressKind.  Some have case
>> labels for all enumeration values, others rely on a default label.
>> Some abort when the value isn't a valid SocketAddressKind, others
>> report an error then.
>>
>> Unify as follows.  Always provide case labels for all enumeration
>> values, to clarify intent.  Abort when the value isn't a valid
>> SocketAddressKind, because the program state is messed up then.
>>
>> Improve a few error messages while there.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  io/dns-resolver.c   |  7 +++++--
>>  ui/vnc.c            | 18 ++++++++++++------
>>  util/qemu-sockets.c |  4 +---
>>  3 files changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/io/dns-resolver.c b/io/dns-resolver.c
>> index 0ac6b23..a407075 100644
>> --- a/io/dns-resolver.c
>> +++ b/io/dns-resolver.c
>> @@ -164,9 +164,12 @@ int qio_dns_resolver_lookup_sync(QIODNSResolver *resolver,
>>                                                  addrs,
>>                                                  errp);
>>  
>> +    case SOCKET_ADDRESS_KIND_FD:
>> +        error_setg(errp, "Unsupported socket address type 'fd'");
>> +        return -1;
>> +
>>      default:
>> -        error_setg(errp, "Unknown socket address kind");
>> -        return -1;
>> +        abort();
>>      }
>>  }
> 
> Just repeating my v1 comments - this needs to be treating KIND_FD as a no-op,
> rather than raising an error.

Do you want to write a patch? Markus is on vacation and since this is
not a regression, dropping this patch from my queue wouldn't do any good
either.

Max


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

Re: [Qemu-devel] [RFC v2 for-2.9 03/10] io vnc sockets: Clean up SocketAddressKind switches

[Daniel P. Berrange]

On Mon, Apr 03, 2017 at 02:50:12PM +0200, Max Reitz wrote:
> On 03.04.2017 13:48, Daniel P. Berrange wrote:
> > On Thu, Mar 30, 2017 at 03:15:00PM +0200, Markus Armbruster wrote:
> >> We have quite a few switches over SocketAddressKind.  Some have case
> >> labels for all enumeration values, others rely on a default label.
> >> Some abort when the value isn't a valid SocketAddressKind, others
> >> report an error then.
> >>
> >> Unify as follows.  Always provide case labels for all enumeration
> >> values, to clarify intent.  Abort when the value isn't a valid
> >> SocketAddressKind, because the program state is messed up then.
> >>
> >> Improve a few error messages while there.
> >>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> Reviewed-by: Max Reitz <mreitz@redhat.com>
> >> ---
> >>  io/dns-resolver.c   |  7 +++++--
> >>  ui/vnc.c            | 18 ++++++++++++------
> >>  util/qemu-sockets.c |  4 +---
> >>  3 files changed, 18 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/io/dns-resolver.c b/io/dns-resolver.c
> >> index 0ac6b23..a407075 100644
> >> --- a/io/dns-resolver.c
> >> +++ b/io/dns-resolver.c
> >> @@ -164,9 +164,12 @@ int qio_dns_resolver_lookup_sync(QIODNSResolver *resolver,
> >>                                                  addrs,
> >>                                                  errp);
> >>  
> >> +    case SOCKET_ADDRESS_KIND_FD:
> >> +        error_setg(errp, "Unsupported socket address type 'fd'");
> >> +        return -1;
> >> +
> >>      default:
> >> -        error_setg(errp, "Unknown socket address kind");
> >> -        return -1;
> >> +        abort();
> >>      }
> >>  }
> > 
> > Just repeating my v1 comments - this needs to be treating KIND_FD as a no-op,
> > rather than raising an error.
> 
> Do you want to write a patch? Markus is on vacation and since this is
> not a regression, dropping this patch from my queue wouldn't do any good
> either.

Ok, i'll do a follow up.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [RFC v2 for-2.9 03/10] io vnc sockets: Clean up SocketAddressKind switches

[Max Reitz]

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

On 03.04.2017 15:05, Daniel P. Berrange wrote:
> On Mon, Apr 03, 2017 at 02:50:12PM +0200, Max Reitz wrote:
>> On 03.04.2017 13:48, Daniel P. Berrange wrote:
>>> On Thu, Mar 30, 2017 at 03:15:00PM +0200, Markus Armbruster wrote:
>>>> We have quite a few switches over SocketAddressKind.  Some have case
>>>> labels for all enumeration values, others rely on a default label.
>>>> Some abort when the value isn't a valid SocketAddressKind, others
>>>> report an error then.
>>>>
>>>> Unify as follows.  Always provide case labels for all enumeration
>>>> values, to clarify intent.  Abort when the value isn't a valid
>>>> SocketAddressKind, because the program state is messed up then.
>>>>
>>>> Improve a few error messages while there.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>  io/dns-resolver.c   |  7 +++++--
>>>>  ui/vnc.c            | 18 ++++++++++++------
>>>>  util/qemu-sockets.c |  4 +---
>>>>  3 files changed, 18 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/io/dns-resolver.c b/io/dns-resolver.c
>>>> index 0ac6b23..a407075 100644
>>>> --- a/io/dns-resolver.c
>>>> +++ b/io/dns-resolver.c
>>>> @@ -164,9 +164,12 @@ int qio_dns_resolver_lookup_sync(QIODNSResolver *resolver,
>>>>                                                  addrs,
>>>>                                                  errp);
>>>>  
>>>> +    case SOCKET_ADDRESS_KIND_FD:
>>>> +        error_setg(errp, "Unsupported socket address type 'fd'");
>>>> +        return -1;
>>>> +
>>>>      default:
>>>> -        error_setg(errp, "Unknown socket address kind");
>>>> -        return -1;
>>>> +        abort();
>>>>      }
>>>>  }
>>>
>>> Just repeating my v1 comments - this needs to be treating KIND_FD as a no-op,
>>> rather than raising an error.
>>
>> Do you want to write a patch? Markus is on vacation and since this is
>> not a regression, dropping this patch from my queue wouldn't do any good
>> either.
> 
> Ok, i'll do a follow up.

Thanks a lot!

Max


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

[Qemu-devel] [RFC v2 for-2.9 04/10] block: Document -drive problematic code and bugs

[Markus Armbruster]

-blockdev and blockdev_add convert their arguments via QObject to
BlockdevOptions for qmp_blockdev_add(), which converts them back to
QObject, then to a flattened QDict.  The QDict's members are typed
according to the QAPI schema.

-drive converts its argument via QemuOpts to a (flat) QDict.  This
QDict's members are all QString.

Thus, the QType of a flat QDict member depends on whether it comes
from -drive or -blockdev/blockdev_add, except when the QAPI type maps
to QString, which is the case for 'str' and enumeration types.

The block layer core extracts generic configuration from the flat
QDict, and the block driver extracts driver-specific configuration.

Both commonly do so by converting (parts of) the flat QDict to
QemuOpts, which turns all values into strings.  Not exactly elegant,
but correct.

However, A few places access the flat QDict directly:

* Most of them access members that are always QString.  Correct.

* bdrv_open_inherit() accesses a boolean, carefully.  Correct.

* nfs_config() uses a QObject input visitor.  Correct only because the
  visited type contains nothing but QStrings.

* nbd_config() and ssh_config() use a QObject input visitor, and the
  visited types contain non-QStrings: InetSocketAddress members
  @numeric, @to, @ipv4, @ipv6.  -drive works as long as you don't try
  to use them (they're all optional).  @to is ignored anyway.

  Reproducer:
  -drive driver=ssh,server.host=h,server.port=22,server.ipv4,path=p
  -drive driver=nbd,server.type=inet,server.data.host=h,server.data.port=22,server.data.ipv4
  both fail with "Invalid parameter type for 'data.ipv4', expected: boolean"

Add suitable comments to all these places.  Mark the buggy ones FIXME.

"Fortunately", -drive's driver-specific options are entirely
undocumented.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c            | 41 +++++++++++++++++++++++++++++++++++++++--
 block/file-posix.c |  6 ++++++
 block/nbd.c        |  8 ++++++++
 block/nfs.c        |  7 +++++++
 block/rbd.c        |  6 ++++++
 block/ssh.c        |  8 ++++++++
 6 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 6e906ec..b3ce23f 100644
--- a/block.c
+++ b/block.c
@@ -1157,6 +1157,12 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
     if (file != NULL) {
         filename = blk_bs(file)->filename;
     } else {
+       /*
+        * Caution: direct use of non-string @options members is
+        * problematic.  When they come from -blockdev or blockdev_add,
+        * members are typed according to the QAPI schema, but when
+        * they come from -drive, they're all QString.
+        */
         filename = qdict_get_try_str(options, "filename");
     }
 
@@ -1324,6 +1330,12 @@ static int bdrv_fill_options(QDict **options, const char *filename,
     BlockDriver *drv = NULL;
     Error *local_err = NULL;
 
+    /*
+     * Caution: direct use of non-string @options members is
+     * problematic.  When they come from -blockdev or blockdev_add,
+     * members are typed according to the QAPI schema, but when they
+     * come from -drive, they're all QString.
+     */
     drvname = qdict_get_try_str(*options, "driver");
     if (drvname) {
         drv = bdrv_find_format(drvname);
@@ -1358,6 +1370,7 @@ static int bdrv_fill_options(QDict **options, const char *filename,
     }
 
     /* Find the right block driver */
+    /* Direct use of @options members is problematic, see note above */
     filename = qdict_get_try_str(*options, "filename");
 
     if (!drvname && protocol) {
@@ -1987,6 +2000,12 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
     qdict_extract_subqdict(parent_options, &options, bdref_key_dot);
     g_free(bdref_key_dot);
 
+    /*
+     * Caution: direct use of non-string @parent_options members is
+     * problematic.  When they come from -blockdev or blockdev_add,
+     * members are typed according to the QAPI schema, but when they
+     * come from -drive, they're all QString.
+     */
     reference = qdict_get_try_str(parent_options, bdref_key);
     if (reference || qdict_haskey(options, "file.filename")) {
         backing_filename[0] = '\0';
@@ -2059,6 +2078,12 @@ bdrv_open_child_bs(const char *filename, QDict *options, const char *bdref_key,
     qdict_extract_subqdict(options, &image_options, bdref_key_dot);
     g_free(bdref_key_dot);
 
+    /*
+     * Caution: direct use of non-string @options members is
+     * problematic.  When they come from -blockdev or blockdev_add,
+     * members are typed according to the QAPI schema, but when they
+     * come from -drive, they're all QString.
+     */
     reference = qdict_get_try_str(options, bdref_key);
     if (!filename && !reference && !qdict_size(image_options)) {
         if (!allow_none) {
@@ -2275,8 +2300,11 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
     }
 
     /* Set the BDRV_O_RDWR and BDRV_O_ALLOW_RDWR flags.
-     * FIXME: we're parsing the QDict to avoid having to create a
-     * QemuOpts just for this, but neither option is optimal. */
+     * Caution: direct use of non-string @options members is
+     * problematic.  When they come from -blockdev or blockdev_add,
+     * members are typed according to the QAPI schema, but when they
+     * come from -drive, they're all QString.
+     */
     if (g_strcmp0(qdict_get_try_str(options, BDRV_OPT_READ_ONLY), "on") &&
         !qdict_get_try_bool(options, BDRV_OPT_READ_ONLY, false)) {
         flags |= (BDRV_O_RDWR | BDRV_O_ALLOW_RDWR);
@@ -2298,6 +2326,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
     options = qdict_clone_shallow(options);
 
     /* Find the right image format driver */
+    /* Direct use of @options members is problematic, see note above */
     drvname = qdict_get_try_str(options, "driver");
     if (drvname) {
         drv = bdrv_find_format(drvname);
@@ -2309,6 +2338,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
 
     assert(drvname || !(flags & BDRV_O_PROTOCOL));
 
+    /* Direct use of @options members is problematic, see note above */
     backing = qdict_get_try_str(options, "backing");
     if (backing && *backing == '\0') {
         flags |= BDRV_O_NO_BACKING;
@@ -2787,6 +2817,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         do {
             QString *new_obj = qobject_to_qstring(entry->value);
             const char *new = qstring_get_str(new_obj);
+           /*
+            * Caution: direct use of non-string bs->options members is
+            * problematic.  When they come from -blockdev or
+            * blockdev_add, members are typed according to the QAPI
+            * schema, but when they come from -drive, they're all
+            * QString.
+            */
             const char *old = qdict_get_try_str(reopen_state->bs->options,
                                                 entry->key);
 
diff --git a/block/file-posix.c b/block/file-posix.c
index 0841a08..738541e 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2193,6 +2193,12 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
     int ret;
 
 #if defined(__APPLE__) && defined(__MACH__)
+    /*
+     * Caution: direct use of non-string @options members is
+     * problematic.  When they come from -blockdev or blockdev_add,
+     * members are typed according to the QAPI schema, but when they
+     * come from -drive, they're all QString.
+     */
     const char *filename = qdict_get_str(options, "filename");
     char bsd_path[MAXPATHLEN] = "";
     bool error_occurred = false;
diff --git a/block/nbd.c b/block/nbd.c
index 36ea617..11e3ba7 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -278,6 +278,14 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, Error **errp)
         goto done;
     }
 
+    /*
+     * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive
+     * server.type=inet.  .to doesn't matter, it's ignored anyway.
+     * That's because when @options come from -blockdev or
+     * blockdev_add, members are typed according to the QAPI schema,
+     * but when they come from -drive, they're all QString.  The
+     * visitor expects the former.
+     */
     iv = qobject_input_visitor_new(crumpled_addr);
     visit_type_SocketAddress(iv, NULL, &saddr, &local_err);
     if (local_err) {
diff --git a/block/nfs.c b/block/nfs.c
index 3f43f6e..42407de 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -474,6 +474,13 @@ static NFSServer *nfs_config(QDict *options, Error **errp)
         goto out;
     }
 
+    /*
+     * Caution: this works only because all scalar members of
+     * NFSServer are QString in @crumpled_addr.  The visitor expects
+     * @crumpled_addr to be typed according to the QAPI scherma.  It
+     * is when @options come from -blockdev or blockdev_add.  But when
+     * they come from -drive, they're all QString.
+     */
     iv = qobject_input_visitor_new(crumpled_addr);
     visit_type_NFSServer(iv, NULL, &server, &local_error);
     if (local_error) {
diff --git a/block/rbd.c b/block/rbd.c
index 498322b..b9a9526 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -384,6 +384,12 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
         goto exit;
     }
 
+    /*
+     * Caution: direct use of non-string @options members is
+     * problematic.  When they come from -blockdev or blockdev_add,
+     * members are typed according to the QAPI schema, but when they
+     * come from -drive, they're all QString.
+     */
     pool       = qdict_get_try_str(options, "pool");
     conf       = qdict_get_try_str(options, "conf");
     clientname = qdict_get_try_str(options, "user");
diff --git a/block/ssh.c b/block/ssh.c
index 278e66f..471ba8a 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -601,6 +601,14 @@ static InetSocketAddress *ssh_config(QDict *options, Error **errp)
         goto out;
     }
 
+    /*
+     * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive.
+     * .to doesn't matter, it's ignored anyway.
+     * That's because when @options come from -blockdev or
+     * blockdev_add, members are typed according to the QAPI schema,
+     * but when they come from -drive, they're all QString.  The
+     * visitor expects the former.
+     */
     iv = qobject_input_visitor_new(crumpled_addr);
     visit_type_InetSocketAddress(iv, NULL, &inet, &local_error);
     if (local_error) {
-- 
2.7.4

Re: [Qemu-devel] [RFC v2 for-2.9 04/10] block: Document -drive problematic code and bugs

[Eric Blake]

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

On 03/30/2017 08:15 AM, Markus Armbruster wrote:

> However, A few places access the flat QDict directly:
> 
> * Most of them access members that are always QString.  Correct.
> 
> * bdrv_open_inherit() accesses a boolean, carefully.  Correct.
> 
> * nfs_config() uses a QObject input visitor.  Correct only because the
>   visited type contains nothing but QStrings.
> 
> * nbd_config() and ssh_config() use a QObject input visitor, and the
>   visited types contain non-QStrings: InetSocketAddress members
>   @numeric, @to, @ipv4, @ipv6.  -drive works as long as you don't try
>   to use them (they're all optional).  @to is ignored anyway.
> 
>   Reproducer:
>   -drive driver=ssh,server.host=h,server.port=22,server.ipv4,path=p
>   -drive driver=nbd,server.type=inet,server.data.host=h,server.data.port=22,server.data.ipv4
>   both fail with "Invalid parameter type for 'data.ipv4', expected: boolean"
> 
> Add suitable comments to all these places.  Mark the buggy ones FIXME.
> 
> "Fortunately", -drive's driver-specific options are entirely
> undocumented.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> +++ b/block.c
> @@ -1157,6 +1157,12 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
>      if (file != NULL) {
>          filename = blk_bs(file)->filename;
>      } else {
> +       /*
> +        * Caution: direct use of non-string @options members is
> +        * problematic.  When they come from -blockdev or blockdev_add,
> +        * members are typed according to the QAPI schema, but when
> +        * they come from -drive, they're all QString.
> +        */
>          filename = qdict_get_try_str(options, "filename");

Did we get the latest round of comment tweaking in here?  I was
expecting to see something along the lines of:

"Caution: accessing 'filename' from @options here is safe, but direct
use of any non-string @options members would be problematic.  When they
come..."

>      }
>  
> @@ -1324,6 +1330,12 @@ static int bdrv_fill_options(QDict **options, const char *filename,
>      BlockDriver *drv = NULL;
>      Error *local_err = NULL;
>  
> +    /*
> +     * Caution: direct use of non-string @options members is
> +     * problematic.  When they come from -blockdev or blockdev_add,
> +     * members are typed according to the QAPI schema, but when they
> +     * come from -drive, they're all QString.
> +     */
>      drvname = qdict_get_try_str(*options, "driver");

Again, wordsmithing might be nice to call out that 'driver' is safe, but
future additions of other accesses must be careful.

> @@ -1987,6 +2000,12 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
>      qdict_extract_subqdict(parent_options, &options, bdref_key_dot);
>      g_free(bdref_key_dot);
>  
> +    /*
> +     * Caution: direct use of non-string @parent_options members is
> +     * problematic.  When they come from -blockdev or blockdev_add,
> +     * members are typed according to the QAPI schema, but when they
> +     * come from -drive, they're all QString.
> +     */
>      reference = qdict_get_try_str(parent_options, bdref_key);

Again, wordsmithing to mention that bdref_key is safe.

>      if (reference || qdict_haskey(options, "file.filename")) {
>          backing_filename[0] = '\0';
> @@ -2059,6 +2078,12 @@ bdrv_open_child_bs(const char *filename, QDict *options, const char *bdref_key,
>      qdict_extract_subqdict(options, &image_options, bdref_key_dot);
>      g_free(bdref_key_dot);
>  
> +    /*
> +     * Caution: direct use of non-string @options members is
> +     * problematic.  When they come from -blockdev or blockdev_add,
> +     * members are typed according to the QAPI schema, but when they
> +     * come from -drive, they're all QString.
> +     */
>      reference = qdict_get_try_str(options, bdref_key);

Ditto.

>      if (!filename && !reference && !qdict_size(image_options)) {
>          if (!allow_none) {
> @@ -2275,8 +2300,11 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>      }
>  
>      /* Set the BDRV_O_RDWR and BDRV_O_ALLOW_RDWR flags.
> -     * FIXME: we're parsing the QDict to avoid having to create a
> -     * QemuOpts just for this, but neither option is optimal. */
> +     * Caution: direct use of non-string @options members is
> +     * problematic.  When they come from -blockdev or blockdev_add,
> +     * members are typed according to the QAPI schema, but when they
> +     * come from -drive, they're all QString.
> +     */
>      if (g_strcmp0(qdict_get_try_str(options, BDRV_OPT_READ_ONLY), "on") &&
>          !qdict_get_try_bool(options, BDRV_OPT_READ_ONLY, false)) {
>          flags |= (BDRV_O_RDWR | BDRV_O_ALLOW_RDWR);

Maybe here, the wordsmithing would mention that the extra hoops we jump
through to cover both types is what makes access safe.

> +++ b/block/nbd.c
> @@ -278,6 +278,14 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, Error **errp)
>          goto done;
>      }
>  
> +    /*
> +     * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive
> +     * server.type=inet.  .to doesn't matter, it's ignored anyway.
> +     * That's because when @options come from -blockdev or
> +     * blockdev_add, members are typed according to the QAPI schema,
> +     * but when they come from -drive, they're all QString.  The
> +     * visitor expects the former.
> +     */

This one is fine.

>      iv = qobject_input_visitor_new(crumpled_addr);
>      visit_type_SocketAddress(iv, NULL, &saddr, &local_err);
>      if (local_err) {
> diff --git a/block/nfs.c b/block/nfs.c
> index 3f43f6e..42407de 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -474,6 +474,13 @@ static NFSServer *nfs_config(QDict *options, Error **errp)
>          goto out;
>      }
>  
> +    /*
> +     * Caution: this works only because all scalar members of
> +     * NFSServer are QString in @crumpled_addr.  The visitor expects
> +     * @crumpled_addr to be typed according to the QAPI scherma.  It
> +     * is when @options come from -blockdev or blockdev_add.  But when
> +     * they come from -drive, they're all QString.
> +     */
>      iv = qobject_input_visitor_new(crumpled_addr);

This one is also fine.

>      visit_type_NFSServer(iv, NULL, &server, &local_error);
>      if (local_error) {
> diff --git a/block/rbd.c b/block/rbd.c
> index 498322b..b9a9526 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -384,6 +384,12 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
>          goto exit;
>      }
>  
> +    /*
> +     * Caution: direct use of non-string @options members is
> +     * problematic.  When they come from -blockdev or blockdev_add,
> +     * members are typed according to the QAPI schema, but when they
> +     * come from -drive, they're all QString.

Here, wordsmithing may be worth mentioning that we are safe because
these are all strings.

> +     */
>      pool       = qdict_get_try_str(options, "pool");
>      conf       = qdict_get_try_str(options, "conf");
>      clientname = qdict_get_try_str(options, "user");
> diff --git a/block/ssh.c b/block/ssh.c
> index 278e66f..471ba8a 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -601,6 +601,14 @@ static InetSocketAddress *ssh_config(QDict *options, Error **errp)
>          goto out;
>      }
>  
> +    /*
> +     * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive.
> +     * .to doesn't matter, it's ignored anyway.
> +     * That's because when @options come from -blockdev or
> +     * blockdev_add, members are typed according to the QAPI schema,
> +     * but when they come from -drive, they're all QString.  The
> +     * visitor expects the former.
> +     */
>      iv = qobject_input_visitor_new(crumpled_addr);

This one's fine.

The overall idea makes sense, but since this is still RFC, and there may
still be wordsmithing to do, I'll wait to give R-b until v3.

-- 
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] [RFC v2 for-2.9 04/10] block: Document -drive problematic code and bugs

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/30/2017 08:15 AM, Markus Armbruster wrote:
>
>> However, A few places access the flat QDict directly:
>> 
>> * Most of them access members that are always QString.  Correct.
>> 
>> * bdrv_open_inherit() accesses a boolean, carefully.  Correct.
>> 
>> * nfs_config() uses a QObject input visitor.  Correct only because the
>>   visited type contains nothing but QStrings.
>> 
>> * nbd_config() and ssh_config() use a QObject input visitor, and the
>>   visited types contain non-QStrings: InetSocketAddress members
>>   @numeric, @to, @ipv4, @ipv6.  -drive works as long as you don't try
>>   to use them (they're all optional).  @to is ignored anyway.
>> 
>>   Reproducer:
>>   -drive driver=ssh,server.host=h,server.port=22,server.ipv4,path=p
>>   -drive driver=nbd,server.type=inet,server.data.host=h,server.data.port=22,server.data.ipv4
>>   both fail with "Invalid parameter type for 'data.ipv4', expected: boolean"
>> 
>> Add suitable comments to all these places.  Mark the buggy ones FIXME.
>> 
>> "Fortunately", -drive's driver-specific options are entirely
>> undocumented.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> +++ b/block.c
>> @@ -1157,6 +1157,12 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
>>      if (file != NULL) {
>>          filename = blk_bs(file)->filename;
>>      } else {
>> +       /*
>> +        * Caution: direct use of non-string @options members is
>> +        * problematic.  When they come from -blockdev or blockdev_add,
>> +        * members are typed according to the QAPI schema, but when
>> +        * they come from -drive, they're all QString.
>> +        */
>>          filename = qdict_get_try_str(options, "filename");
>
> Did we get the latest round of comment tweaking in here?  I was
> expecting to see something along the lines of:
>
> "Caution: accessing 'filename' from @options here is safe, but direct
> use of any non-string @options members would be problematic.  When they
> come..."

I haven't updated this patch for review, yet.  One more reason this is
RFC.  Forgot to mention in the cover letter, sorry.

>>      }
>>  
>> @@ -1324,6 +1330,12 @@ static int bdrv_fill_options(QDict **options, const char *filename,
>>      BlockDriver *drv = NULL;
>>      Error *local_err = NULL;
>>  
>> +    /*
>> +     * Caution: direct use of non-string @options members is
>> +     * problematic.  When they come from -blockdev or blockdev_add,
>> +     * members are typed according to the QAPI schema, but when they
>> +     * come from -drive, they're all QString.
>> +     */
>>      drvname = qdict_get_try_str(*options, "driver");
>
> Again, wordsmithing might be nice to call out that 'driver' is safe, but
> future additions of other accesses must be careful.

Yes.

>> @@ -1987,6 +2000,12 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
>>      qdict_extract_subqdict(parent_options, &options, bdref_key_dot);
>>      g_free(bdref_key_dot);
>>  
>> +    /*
>> +     * Caution: direct use of non-string @parent_options members is
>> +     * problematic.  When they come from -blockdev or blockdev_add,
>> +     * members are typed according to the QAPI schema, but when they
>> +     * come from -drive, they're all QString.
>> +     */
>>      reference = qdict_get_try_str(parent_options, bdref_key);
>
> Again, wordsmithing to mention that bdref_key is safe.

Yes.

>>      if (reference || qdict_haskey(options, "file.filename")) {
>>          backing_filename[0] = '\0';
>> @@ -2059,6 +2078,12 @@ bdrv_open_child_bs(const char *filename, QDict *options, const char *bdref_key,
>>      qdict_extract_subqdict(options, &image_options, bdref_key_dot);
>>      g_free(bdref_key_dot);
>>  
>> +    /*
>> +     * Caution: direct use of non-string @options members is
>> +     * problematic.  When they come from -blockdev or blockdev_add,
>> +     * members are typed according to the QAPI schema, but when they
>> +     * come from -drive, they're all QString.
>> +     */
>>      reference = qdict_get_try_str(options, bdref_key);
>
> Ditto.

Yes.

>>      if (!filename && !reference && !qdict_size(image_options)) {
>>          if (!allow_none) {
>> @@ -2275,8 +2300,11 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>>      }
>>  
>>      /* Set the BDRV_O_RDWR and BDRV_O_ALLOW_RDWR flags.
>> -     * FIXME: we're parsing the QDict to avoid having to create a
>> -     * QemuOpts just for this, but neither option is optimal. */
>> +     * Caution: direct use of non-string @options members is
>> +     * problematic.  When they come from -blockdev or blockdev_add,
>> +     * members are typed according to the QAPI schema, but when they
>> +     * come from -drive, they're all QString.
>> +     */
>>      if (g_strcmp0(qdict_get_try_str(options, BDRV_OPT_READ_ONLY), "on") &&
>>          !qdict_get_try_bool(options, BDRV_OPT_READ_ONLY, false)) {
>>          flags |= (BDRV_O_RDWR | BDRV_O_ALLOW_RDWR);
>
> Maybe here, the wordsmithing would mention that the extra hoops we jump
> through to cover both types is what makes access safe.

Yes.

>> +++ b/block/nbd.c
>> @@ -278,6 +278,14 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, Error **errp)
>>          goto done;
>>      }
>>  
>> +    /*
>> +     * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive
>> +     * server.type=inet.  .to doesn't matter, it's ignored anyway.
>> +     * That's because when @options come from -blockdev or
>> +     * blockdev_add, members are typed according to the QAPI schema,
>> +     * but when they come from -drive, they're all QString.  The
>> +     * visitor expects the former.
>> +     */
>
> This one is fine.
>
>>      iv = qobject_input_visitor_new(crumpled_addr);
>>      visit_type_SocketAddress(iv, NULL, &saddr, &local_err);
>>      if (local_err) {
>> diff --git a/block/nfs.c b/block/nfs.c
>> index 3f43f6e..42407de 100644
>> --- a/block/nfs.c
>> +++ b/block/nfs.c
>> @@ -474,6 +474,13 @@ static NFSServer *nfs_config(QDict *options, Error **errp)
>>          goto out;
>>      }
>>  
>> +    /*
>> +     * Caution: this works only because all scalar members of
>> +     * NFSServer are QString in @crumpled_addr.  The visitor expects
>> +     * @crumpled_addr to be typed according to the QAPI scherma.  It
>> +     * is when @options come from -blockdev or blockdev_add.  But when
>> +     * they come from -drive, they're all QString.
>> +     */
>>      iv = qobject_input_visitor_new(crumpled_addr);
>
> This one is also fine.
>
>>      visit_type_NFSServer(iv, NULL, &server, &local_error);
>>      if (local_error) {
>> diff --git a/block/rbd.c b/block/rbd.c
>> index 498322b..b9a9526 100644
>> --- a/block/rbd.c
>> +++ b/block/rbd.c
>> @@ -384,6 +384,12 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
>>          goto exit;
>>      }
>>  
>> +    /*
>> +     * Caution: direct use of non-string @options members is
>> +     * problematic.  When they come from -blockdev or blockdev_add,
>> +     * members are typed according to the QAPI schema, but when they
>> +     * come from -drive, they're all QString.
>
> Here, wordsmithing may be worth mentioning that we are safe because
> these are all strings.

Yes.

>> +     */
>>      pool       = qdict_get_try_str(options, "pool");
>>      conf       = qdict_get_try_str(options, "conf");
>>      clientname = qdict_get_try_str(options, "user");
>> diff --git a/block/ssh.c b/block/ssh.c
>> index 278e66f..471ba8a 100644
>> --- a/block/ssh.c
>> +++ b/block/ssh.c
>> @@ -601,6 +601,14 @@ static InetSocketAddress *ssh_config(QDict *options, Error **errp)
>>          goto out;
>>      }
>>  
>> +    /*
>> +     * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive.
>> +     * .to doesn't matter, it's ignored anyway.
>> +     * That's because when @options come from -blockdev or
>> +     * blockdev_add, members are typed according to the QAPI schema,
>> +     * but when they come from -drive, they're all QString.  The
>> +     * visitor expects the former.
>> +     */
>>      iv = qobject_input_visitor_new(crumpled_addr);
>
> This one's fine.
>
> The overall idea makes sense, but since this is still RFC, and there may
> still be wordsmithing to do, I'll wait to give R-b until v3.

Thanks!

[Qemu-devel] [RFC v2 for-2.9 05/10] gluster: Prepare for SocketAddressFlat extension

[Markus Armbruster]

qemu_gluster_glfs_init() and qemu_gluster_parse_json() rely on the
fact that SocketAddressFlatType has only two members
SOCKET_ADDRESS_FLAT_TYPE_INET and SOCKET_ADDRESS_FLAT_TYPE_UNIX.
Correct, but won't stay correct.  Make them more robust.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
---
 block/gluster.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index a577dae..fb0aafe 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -412,10 +412,12 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
     glfs_set_preopened(gconf->volume, glfs);
 
     for (server = gconf->server; server; server = server->next) {
-        if (server->value->type  == SOCKET_ADDRESS_FLAT_TYPE_UNIX) {
+        switch (server->value->type) {
+        case SOCKET_ADDRESS_FLAT_TYPE_UNIX:
             ret = glfs_set_volfile_server(glfs, "unix",
                                    server->value->u.q_unix.path, 0);
-        } else {
+            break;
+        case SOCKET_ADDRESS_FLAT_TYPE_INET:
             if (parse_uint_full(server->value->u.inet.port, &port, 10) < 0 ||
                 port > 65535) {
                 error_setg(errp, "'%s' is not a valid port number",
@@ -426,6 +428,9 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
             ret = glfs_set_volfile_server(glfs, "tcp",
                                    server->value->u.inet.host,
                                    (int)port);
+            break;
+        default:
+            abort();
         }
 
         if (ret < 0) {
@@ -487,7 +492,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
     char *str = NULL;
     const char *ptr;
     size_t num_servers;
-    int i;
+    int i, type;
 
     /* create opts info from runtime_json_opts list */
     opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
@@ -539,16 +544,17 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
         if (!strcmp(ptr, "tcp")) {
             ptr = "inet";       /* accept legacy "tcp" */
         }
-        gsconf->type = qapi_enum_parse(SocketAddressFlatType_lookup, ptr,
-                                       SOCKET_ADDRESS_FLAT_TYPE__MAX, -1,
-                                       &local_err);
-        if (local_err) {
-            error_append_hint(&local_err,
-                              "Parameter '%s' may be 'inet' or 'unix'\n",
-                              GLUSTER_OPT_TYPE);
+        type = qapi_enum_parse(SocketAddressFlatType_lookup, ptr,
+                               SOCKET_ADDRESS_FLAT_TYPE__MAX, -1, NULL);
+        if (type != SOCKET_ADDRESS_FLAT_TYPE_INET
+            && type != SOCKET_ADDRESS_FLAT_TYPE_UNIX) {
+            error_setg(&local_err,
+                       "Parameter '%s' may be 'inet' or 'unix'",
+                       GLUSTER_OPT_TYPE);
             error_append_hint(&local_err, GERR_INDEX_HINT, i);
             goto out;
         }
+        gsconf->type = type;
         qemu_opts_del(opts);
 
         if (gsconf->type == SOCKET_ADDRESS_FLAT_TYPE_INET) {
-- 
2.7.4

[Qemu-devel] [RFC v2 for-2.9 06/10] qapi-schema: SocketAddressFlat variants 'vsock' and 'fd'

[Markus Armbruster]

Note that the new variants are impossible in qemu_gluster_glfs_init(),
because the gconf->server can only come from qemu_gluster_parse_uri()
or qemu_gluster_parse_json(), and neither can create anything but
'inet' or 'unix'.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/gluster.c  |  2 ++
 qapi-schema.json | 19 ++++++++-----------
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index fb0aafe..cf29b5f 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -429,6 +429,8 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
                                    server->value->u.inet.host,
                                    (int)port);
             break;
+        case SOCKET_ADDRESS_FLAT_TYPE_VSOCK:
+        case SOCKET_ADDRESS_FLAT_TYPE_FD:
         default:
             abort();
         }
diff --git a/qapi-schema.json b/qapi-schema.json
index b921994..927c9a2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4144,7 +4144,7 @@
 # Since: 2.9
 ##
 { 'enum': 'SocketAddressFlatType',
-  'data': [ 'unix', 'inet' ] }
+  'data': [ 'inet', 'unix', 'vsock', 'fd' ] }
 
 ##
 # @SocketAddressFlat:
@@ -4153,22 +4153,19 @@
 #
 # @type:       Transport type
 #
-# This is similar to SocketAddress, only distinction:
-#
-# 1. SocketAddressFlat is a flat union, SocketAddress is a simple union.
-#    A flat union is nicer than simple because it avoids nesting
-#    (i.e. more {}) on the wire.
-#
-# 2. SocketAddressFlat supports only types 'unix' and 'inet', because
-#    that's what its current users need.
+# This is just like SocketAddress, except it's a flat union rather
+# than a simple union.  Nicer because it avoids nesting (i.e. more {})
+# on the wire.
 #
 # Since: 2.9
 ##
 { 'union': 'SocketAddressFlat',
   'base': { 'type': 'SocketAddressFlatType' },
   'discriminator': 'type',
-  'data': { 'unix': 'UnixSocketAddress',
-            'inet': 'InetSocketAddress' } }
+  'data': { 'inet': 'InetSocketAddress',
+            'unix': 'UnixSocketAddress',
+            'vsock': 'VsockSocketAddress',
+            'fd': 'String' } }
 
 ##
 # @getfd:
-- 
2.7.4

Re: [Qemu-devel] [RFC v2 for-2.9 06/10] qapi-schema: SocketAddressFlat variants 'vsock' and 'fd'

[Eric Blake]

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

On 03/30/2017 08:15 AM, Markus Armbruster wrote:
> Note that the new variants are impossible in qemu_gluster_glfs_init(),
> because the gconf->server can only come from qemu_gluster_parse_uri()
> or qemu_gluster_parse_json(), and neither can create anything but
> 'inet' or 'unix'.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/gluster.c  |  2 ++
>  qapi-schema.json | 19 ++++++++-----------
>  2 files changed, 10 insertions(+), 11 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>


> -# 2. SocketAddressFlat supports only types 'unix' and 'inet', because
> -#    that's what its current users need.
> +# This is just like SocketAddress, except it's a flat union rather
> +# than a simple union.  Nicer because it avoids nesting (i.e. more {})
> +# on the wire.

I know that you are explaining that it is "nesting" which means "more
{}", but it is SocketAddress that has the extra {}, not
SocketAddressFlat.  I wonder if it reads any better as:

Nicer because it avoids nesting on the wire (i.e. this form has fewer {}).

-- 
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] [RFC v2 for-2.9 06/10] qapi-schema: SocketAddressFlat variants 'vsock' and 'fd'

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/30/2017 08:15 AM, Markus Armbruster wrote:
>> Note that the new variants are impossible in qemu_gluster_glfs_init(),
>> because the gconf->server can only come from qemu_gluster_parse_uri()
>> or qemu_gluster_parse_json(), and neither can create anything but
>> 'inet' or 'unix'.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/gluster.c  |  2 ++
>>  qapi-schema.json | 19 ++++++++-----------
>>  2 files changed, 10 insertions(+), 11 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>
>> -# 2. SocketAddressFlat supports only types 'unix' and 'inet', because
>> -#    that's what its current users need.
>> +# This is just like SocketAddress, except it's a flat union rather
>> +# than a simple union.  Nicer because it avoids nesting (i.e. more {})
>> +# on the wire.
>
> I know that you are explaining that it is "nesting" which means "more
> {}", but it is SocketAddress that has the extra {}, not
> SocketAddressFlat.  I wonder if it reads any better as:
>
> Nicer because it avoids nesting on the wire (i.e. this form has fewer {}).

Sold!  (Except I'm peeling off the parenthesis)

[Qemu-devel] [RFC v2 for-2.9 07/10] sockets: New helper socket_address_crumple()

[Markus Armbruster]

SocketAddress is a simple union, and simple unions are awkward: they
have their variant members wrapped in a "data" object on the wire, and
require additional indirections in C.  I intend to limit its use to
existing external interfaces.  New ones should use SocketAddressFlat.
I further intend to convert all internal interfaces to
SocketAddressFlat.  This helper should go away then.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qemu/sockets.h | 11 +++++++++++
 util/qemu-sockets.c    | 29 +++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 5f1bab9..cef05a5 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -119,4 +119,15 @@ SocketAddress *socket_remote_address(int fd, Error **errp);
  */
 char *socket_address_to_string(struct SocketAddress *addr, Error **errp);
 
+/**
+ * socket_address_crumple:
+ * @addr_flat: the socket addres to crumple
+ *
+ * Convert SocketAddressFlat to SocketAddress.  Caller is responsible
+ * for freeing with qapi_free_SocketAddress().
+ *
+ * Returns: the argument converted to SocketAddress.
+ */
+SocketAddress *socket_address_crumple(SocketAddressFlat *addr_flat);
+
 #endif /* QEMU_SOCKETS_H */
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 4ae37bd..03a2a47 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -25,6 +25,7 @@
 #include "qapi/error.h"
 #include "qemu/sockets.h"
 #include "qemu/main-loop.h"
+#include "qapi/clone-visitor.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qobject-output-visitor.h"
 #include "qapi-visit.h"
@@ -1341,3 +1342,31 @@ char *socket_address_to_string(struct SocketAddress *addr, Error **errp)
     }
     return buf;
 }
+
+SocketAddress *socket_address_crumple(SocketAddressFlat *addr_flat)
+{
+    SocketAddress *addr = g_new(SocketAddress, 1);
+
+    addr->type = addr_flat->type;
+    switch (addr->type) {
+    case SOCKET_ADDRESS_FLAT_TYPE_INET:
+        addr->u.inet.data = QAPI_CLONE(InetSocketAddress,
+                                        &addr_flat->u.inet);
+        break;
+    case SOCKET_ADDRESS_FLAT_TYPE_UNIX:
+        addr->u.q_unix.data = QAPI_CLONE(UnixSocketAddress, &
+                                          addr_flat->u.q_unix);
+        break;
+    case SOCKET_ADDRESS_FLAT_TYPE_VSOCK:
+        addr->u.vsock.data = QAPI_CLONE(VsockSocketAddress,
+                                         &addr_flat->u.vsock);
+        break;
+    case SOCKET_ADDRESS_FLAT_TYPE_FD:
+        addr->u.fd.data = QAPI_CLONE(String, &addr_flat->u.fd);
+        break;
+    default:
+        abort();
+    }
+
+    return addr;
+}
-- 
2.7.4

Re: [Qemu-devel] [RFC v2 for-2.9 07/10] sockets: New helper socket_address_crumple()

[Eric Blake]

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

On 03/30/2017 08:15 AM, Markus Armbruster wrote:
> SocketAddress is a simple union, and simple unions are awkward: they
> have their variant members wrapped in a "data" object on the wire, and
> require additional indirections in C.  I intend to limit its use to
> existing external interfaces.  New ones should use SocketAddressFlat.
> I further intend to convert all internal interfaces to
> SocketAddressFlat.  This helper should go away then.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qemu/sockets.h | 11 +++++++++++
>  util/qemu-sockets.c    | 29 +++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 5f1bab9..cef05a5 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -119,4 +119,15 @@ SocketAddress *socket_remote_address(int fd, Error **errp);
>   */
>  char *socket_address_to_string(struct SocketAddress *addr, Error **errp);
>  
> +/**
> + * socket_address_crumple:
> + * @addr_flat: the socket addres to crumple

s/addres/address/

> +SocketAddress *socket_address_crumple(SocketAddressFlat *addr_flat)
> +{
> +    SocketAddress *addr = g_new(SocketAddress, 1);
> +
> +    addr->type = addr_flat->type;

Works only because our enum is defined in the same order as the simple
union's members.  A bit fragile, so maybe we want to comment in the
.json file that we can't reorder members of either the enum or the
simple union's 'data'?  Or it might even tickle a picky compiler to warn
about assignment between incompatible enum types.  Another option would
be making it robust by instead doing switch(addr_flat->type) and
assigning to addr->type in each branch of the switch.

> +    switch (addr->type) {
> +    case SOCKET_ADDRESS_FLAT_TYPE_INET:
> +        addr->u.inet.data = QAPI_CLONE(InetSocketAddress,
> +                                        &addr_flat->u.inet);

Indentation is off.

> +        break;
> +    case SOCKET_ADDRESS_FLAT_TYPE_UNIX:
> +        addr->u.q_unix.data = QAPI_CLONE(UnixSocketAddress, &
> +                                          addr_flat->u.q_unix);

Why is the unary & split from its argument?

> +        break;
> +    case SOCKET_ADDRESS_FLAT_TYPE_VSOCK:
> +        addr->u.vsock.data = QAPI_CLONE(VsockSocketAddress,
> +                                         &addr_flat->u.vsock);

More indentation problems.

> +        break;
> +    case SOCKET_ADDRESS_FLAT_TYPE_FD:
> +        addr->u.fd.data = QAPI_CLONE(String, &addr_flat->u.fd);
> +        break;
> +    default:
> +        abort();
> +    }
> +
> +    return addr;
> +}
> 

-- 
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] [RFC v2 for-2.9 07/10] sockets: New helper socket_address_crumple()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/30/2017 08:15 AM, Markus Armbruster wrote:
>> SocketAddress is a simple union, and simple unions are awkward: they
>> have their variant members wrapped in a "data" object on the wire, and
>> require additional indirections in C.  I intend to limit its use to
>> existing external interfaces.  New ones should use SocketAddressFlat.
>> I further intend to convert all internal interfaces to
>> SocketAddressFlat.  This helper should go away then.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/qemu/sockets.h | 11 +++++++++++
>>  util/qemu-sockets.c    | 29 +++++++++++++++++++++++++++++
>>  2 files changed, 40 insertions(+)
>> 
>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
>> index 5f1bab9..cef05a5 100644
>> --- a/include/qemu/sockets.h
>> +++ b/include/qemu/sockets.h
>> @@ -119,4 +119,15 @@ SocketAddress *socket_remote_address(int fd, Error **errp);
>>   */
>>  char *socket_address_to_string(struct SocketAddress *addr, Error **errp);
>>  
>> +/**
>> + * socket_address_crumple:
>> + * @addr_flat: the socket addres to crumple
>
> s/addres/address/

Fixing...

>> +SocketAddress *socket_address_crumple(SocketAddressFlat *addr_flat)
>> +{
>> +    SocketAddress *addr = g_new(SocketAddress, 1);
>> +
>> +    addr->type = addr_flat->type;
>
> Works only because our enum is defined in the same order as the simple
> union's members.  A bit fragile, so maybe we want to comment in the
> .json file that we can't reorder members of either the enum or the
> simple union's 'data'?  Or it might even tickle a picky compiler to warn
> about assignment between incompatible enum types.  Another option would
> be making it robust by instead doing switch(addr_flat->type) and
> assigning to addr->type in each branch of the switch.

Sold.

>> +    switch (addr->type) {
>> +    case SOCKET_ADDRESS_FLAT_TYPE_INET:
>> +        addr->u.inet.data = QAPI_CLONE(InetSocketAddress,
>> +                                        &addr_flat->u.inet);
>
> Indentation is off.

>> +        break;
>> +    case SOCKET_ADDRESS_FLAT_TYPE_UNIX:
>> +        addr->u.q_unix.data = QAPI_CLONE(UnixSocketAddress, &
>> +                                          addr_flat->u.q_unix);
>
> Why is the unary & split from its argument?

Editing accident.

>> +        break;
>> +    case SOCKET_ADDRESS_FLAT_TYPE_VSOCK:
>> +        addr->u.vsock.data = QAPI_CLONE(VsockSocketAddress,
>> +                                         &addr_flat->u.vsock);
>
> More indentation problems.
>
>> +        break;
>> +    case SOCKET_ADDRESS_FLAT_TYPE_FD:
>> +        addr->u.fd.data = QAPI_CLONE(String, &addr_flat->u.fd);
>> +        break;
>> +    default:
>> +        abort();
>> +    }
>> +
>> +    return addr;
>> +}
>> 

Now looks like this:

SocketAddress *socket_address_crumple(SocketAddressFlat *addr_flat)
{
    SocketAddress *addr = g_new(SocketAddress, 1);

    switch (addr_flat->type) {
    case SOCKET_ADDRESS_FLAT_TYPE_INET:
        addr->type = SOCKET_ADDRESS_KIND_INET;
        addr->u.inet.data = QAPI_CLONE(InetSocketAddress,
                                       &addr_flat->u.inet);
        break;
    case SOCKET_ADDRESS_FLAT_TYPE_UNIX:
        addr->type = SOCKET_ADDRESS_KIND_UNIX;
        addr->u.q_unix.data = QAPI_CLONE(UnixSocketAddress,
                                         &addr_flat->u.q_unix);
        break;
    case SOCKET_ADDRESS_FLAT_TYPE_VSOCK:
        addr->type = SOCKET_ADDRESS_KIND_VSOCK;
        addr->u.vsock.data = QAPI_CLONE(VsockSocketAddress,
                                        &addr_flat->u.vsock);
        break;
    case SOCKET_ADDRESS_FLAT_TYPE_FD:
        addr->type = SOCKET_ADDRESS_KIND_FD;
        addr->u.fd.data = QAPI_CLONE(String, &addr_flat->u.fd);
        break;
    default:
        abort();
    }

    return addr;
}

Re: [Qemu-devel] [RFC v2 for-2.9 07/10] sockets: New helper socket_address_crumple()

[Eric Blake]

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

On 03/30/2017 10:03 AM, Markus Armbruster wrote:

>>> +SocketAddress *socket_address_crumple(SocketAddressFlat *addr_flat)
>>> +{
>>> +    SocketAddress *addr = g_new(SocketAddress, 1);
>>> +
>>> +    addr->type = addr_flat->type;
>>
>> Works only because our enum is defined in the same order as the simple
>> union's members.  A bit fragile, so maybe we want to comment in the
>> .json file that we can't reorder members of either the enum or the
>> simple union's 'data'?  Or it might even tickle a picky compiler to warn
>> about assignment between incompatible enum types.  Another option would
>> be making it robust by instead doing switch(addr_flat->type) and
>> assigning to addr->type in each branch of the switch.
> 
> Sold.
> 

> Now looks like this:
> 
> SocketAddress *socket_address_crumple(SocketAddressFlat *addr_flat)
> {
>     SocketAddress *addr = g_new(SocketAddress, 1);
> 
>     switch (addr_flat->type) {
>     case SOCKET_ADDRESS_FLAT_TYPE_INET:
>         addr->type = SOCKET_ADDRESS_KIND_INET;
>         addr->u.inet.data = QAPI_CLONE(InetSocketAddress,
>                                        &addr_flat->u.inet);
>         break;

Yes, that's better, and no tweaks to the .json file needed.

-- 
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] [RFC v2 for-2.9 07/10] sockets: New helper socket_address_crumple()

[Max Reitz]

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

On 30.03.2017 17:03, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 03/30/2017 08:15 AM, Markus Armbruster wrote:
>>> SocketAddress is a simple union, and simple unions are awkward: they
>>> have their variant members wrapped in a "data" object on the wire, and
>>> require additional indirections in C.  I intend to limit its use to
>>> existing external interfaces.  New ones should use SocketAddressFlat.
>>> I further intend to convert all internal interfaces to
>>> SocketAddressFlat.  This helper should go away then.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  include/qemu/sockets.h | 11 +++++++++++
>>>  util/qemu-sockets.c    | 29 +++++++++++++++++++++++++++++
>>>  2 files changed, 40 insertions(+)
>>>
>>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
>>> index 5f1bab9..cef05a5 100644
>>> --- a/include/qemu/sockets.h
>>> +++ b/include/qemu/sockets.h
>>> @@ -119,4 +119,15 @@ SocketAddress *socket_remote_address(int fd, Error **errp);
>>>   */
>>>  char *socket_address_to_string(struct SocketAddress *addr, Error **errp);
>>>  
>>> +/**
>>> + * socket_address_crumple:
>>> + * @addr_flat: the socket addres to crumple
>>
>> s/addres/address/
> 
> Fixing...
> 
>>> +SocketAddress *socket_address_crumple(SocketAddressFlat *addr_flat)
>>> +{
>>> +    SocketAddress *addr = g_new(SocketAddress, 1);
>>> +
>>> +    addr->type = addr_flat->type;
>>
>> Works only because our enum is defined in the same order as the simple
>> union's members.  A bit fragile, so maybe we want to comment in the
>> .json file that we can't reorder members of either the enum or the
>> simple union's 'data'?  Or it might even tickle a picky compiler to warn
>> about assignment between incompatible enum types.  Another option would
>> be making it robust by instead doing switch(addr_flat->type) and
>> assigning to addr->type in each branch of the switch.
> 
> Sold.
> 
>>> +    switch (addr->type) {
>>> +    case SOCKET_ADDRESS_FLAT_TYPE_INET:
>>> +        addr->u.inet.data = QAPI_CLONE(InetSocketAddress,
>>> +                                        &addr_flat->u.inet);
>>
>> Indentation is off.
> 
>>> +        break;
>>> +    case SOCKET_ADDRESS_FLAT_TYPE_UNIX:
>>> +        addr->u.q_unix.data = QAPI_CLONE(UnixSocketAddress, &
>>> +                                          addr_flat->u.q_unix);
>>
>> Why is the unary & split from its argument?
> 
> Editing accident.
> 
>>> +        break;
>>> +    case SOCKET_ADDRESS_FLAT_TYPE_VSOCK:
>>> +        addr->u.vsock.data = QAPI_CLONE(VsockSocketAddress,
>>> +                                         &addr_flat->u.vsock);
>>
>> More indentation problems.
>>
>>> +        break;
>>> +    case SOCKET_ADDRESS_FLAT_TYPE_FD:
>>> +        addr->u.fd.data = QAPI_CLONE(String, &addr_flat->u.fd);
>>> +        break;
>>> +    default:
>>> +        abort();
>>> +    }
>>> +
>>> +    return addr;
>>> +}
>>>
> 
> Now looks like this:
> 
> SocketAddress *socket_address_crumple(SocketAddressFlat *addr_flat)
> {
>     SocketAddress *addr = g_new(SocketAddress, 1);
> 
>     switch (addr_flat->type) {
>     case SOCKET_ADDRESS_FLAT_TYPE_INET:
>         addr->type = SOCKET_ADDRESS_KIND_INET;
>         addr->u.inet.data = QAPI_CLONE(InetSocketAddress,
>                                        &addr_flat->u.inet);
>         break;
>     case SOCKET_ADDRESS_FLAT_TYPE_UNIX:
>         addr->type = SOCKET_ADDRESS_KIND_UNIX;
>         addr->u.q_unix.data = QAPI_CLONE(UnixSocketAddress,
>                                          &addr_flat->u.q_unix);
>         break;
>     case SOCKET_ADDRESS_FLAT_TYPE_VSOCK:
>         addr->type = SOCKET_ADDRESS_KIND_VSOCK;
>         addr->u.vsock.data = QAPI_CLONE(VsockSocketAddress,
>                                         &addr_flat->u.vsock);
>         break;
>     case SOCKET_ADDRESS_FLAT_TYPE_FD:
>         addr->type = SOCKET_ADDRESS_KIND_FD;
>         addr->u.fd.data = QAPI_CLONE(String, &addr_flat->u.fd);
>         break;
>     default:
>         abort();
>     }
> 
>     return addr;
> }

Looks good!

Max


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

[Qemu-devel] [RFC v2 for-2.9 08/10] nbd: Tidy up blockdev-add interface

[Markus Armbruster]

SocketAddress is a simple union, and simple unions are awkward: they
have their variant members wrapped in a "data" object on the wire, and
require additional indirections in C.  I intend to limit its use to
existing external interfaces, and convert all internal interfaces to
SocketAddressFlat.

BlockdevOptionsNbd is an external interface using SocketAddress.  We
already use SocketAddressFlat elsewhere in blockdev=add.  Replace it
by SocketAddressFlat while we can (it's new in 2.9) for simplicity and
consistency.  For example,

    { "execute": "blockdev-add",
      "arguments": { "node-name": "foo", "driver": "nbd",
                     "server": { "type": "inet",
		                 "data": { "host": "localhost",
				           "port": "12345" } } } }

becomes

    { "execute": "blockdev-add",
      "arguments": { "node-name": "foo", "driver": "nbd",
                     "server": { "type": "inet",
		                 "host": "localhost", "port": "12345" } } }

Since the internal interfaces still take SocketAddress, this requires
conversion function socket_address_crumple().  It'll go away when I
update the interfaces.

Unfortunately, SocketAddress is also visible in -drive since 2.8.  Add
still more gunk to nbd_process_legacy_socket_options().  You can now
shorten

    -drive if=none,driver=nbd,server.type=inet,server.data.host=127.0.0.1,server.data.port=12345

to

    -drive if=none,driver=nbd,server.type=inet,server.host=127.0.0.1,server.port=12345

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/nbd.c          | 94 +++++++++++++++++++++++++++++++++++++---------------
 qapi/block-core.json |  2 +-
 2 files changed, 69 insertions(+), 27 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 11e3ba7..ea9d8dc 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -47,7 +47,7 @@ typedef struct BDRVNBDState {
     NBDClientSession client;
 
     /* For nbd_refresh_filename() */
-    SocketAddress *saddr;
+    SocketAddressFlat *saddr;
     char *export, *tlscredsid;
 } BDRVNBDState;
 
@@ -95,7 +95,7 @@ static int nbd_parse_uri(const char *filename, QDict *options)
             goto out;
         }
         qdict_put(options, "server.type", qstring_from_str("unix"));
-        qdict_put(options, "server.data.path",
+        qdict_put(options, "server.path",
                   qstring_from_str(qp->p[0].value));
     } else {
         QString *host;
@@ -116,10 +116,10 @@ static int nbd_parse_uri(const char *filename, QDict *options)
         }
 
         qdict_put(options, "server.type", qstring_from_str("inet"));
-        qdict_put(options, "server.data.host", host);
+        qdict_put(options, "server.host", host);
 
         port_str = g_strdup_printf("%d", uri->port ?: NBD_DEFAULT_PORT);
-        qdict_put(options, "server.data.port", qstring_from_str(port_str));
+        qdict_put(options, "server.port", qstring_from_str(port_str));
         g_free(port_str);
     }
 
@@ -197,7 +197,7 @@ static void nbd_parse_filename(const char *filename, QDict *options,
     /* are we a UNIX or TCP socket? */
     if (strstart(host_spec, "unix:", &unixpath)) {
         qdict_put(options, "server.type", qstring_from_str("unix"));
-        qdict_put(options, "server.data.path", qstring_from_str(unixpath));
+        qdict_put(options, "server.path", qstring_from_str(unixpath));
     } else {
         InetSocketAddress *addr = NULL;
 
@@ -207,8 +207,8 @@ static void nbd_parse_filename(const char *filename, QDict *options,
         }
 
         qdict_put(options, "server.type", qstring_from_str("inet"));
-        qdict_put(options, "server.data.host", qstring_from_str(addr->host));
-        qdict_put(options, "server.data.port", qstring_from_str(addr->port));
+        qdict_put(options, "server.host", qstring_from_str(addr->host));
+        qdict_put(options, "server.port", qstring_from_str(addr->port));
         qapi_free_InetSocketAddress(addr);
     }
 
@@ -223,12 +223,51 @@ static bool nbd_process_legacy_socket_options(QDict *output_options,
     const char *path = qemu_opt_get(legacy_opts, "path");
     const char *host = qemu_opt_get(legacy_opts, "host");
     const char *port = qemu_opt_get(legacy_opts, "port");
+    const char *sd_path = qdict_get_try_str(output_options,
+                                            "server.data.path");
+    const char *sd_host = qdict_get_try_str(output_options,
+                                            "server.data.host");
+    const char *sd_port = qdict_get_try_str(output_options,
+                                            "server.data.port");
+    bool bare = path || host || port;
+    bool server_data = sd_path || sd_host || sd_port;
+    QObject *val;
     const QDictEntry *e;
 
-    if (!path && !host && !port) {
+    if (!bare && !server_data) {
         return true;
     }
 
+    if (bare && server_data) {
+        error_setg(errp, "Cannot use 'server' and path/host/port at the "
+                   "same time");
+        return false;
+    }
+
+    if (server_data) {
+        if (sd_host) {
+            val = qdict_get(output_options, "server.data.host");
+            qobject_incref(val);
+            qdict_put_obj(output_options, "server.host", val);
+            qdict_del(output_options, "server.data.host");
+        }
+        if (sd_port) {
+            val = qdict_get(output_options, "server.data.port");
+            qobject_incref(val);
+            qdict_put_obj(output_options, "server.port", val);
+            qdict_del(output_options, "server.data.port");
+        }
+        if (sd_path) {
+            val = qdict_get(output_options, "server.data.path");
+            qobject_incref(val);
+            qdict_put_obj(output_options, "server.path", val);
+            qdict_del(output_options, "server.data.path");
+        }
+        return true;
+    }
+
+    assert(bare);
+
     for (e = qdict_first(output_options); e; e = qdict_next(output_options, e))
     {
         if (strstart(e->key, "server.", NULL)) {
@@ -248,20 +287,21 @@ static bool nbd_process_legacy_socket_options(QDict *output_options,
         }
 
         qdict_put(output_options, "server.type", qstring_from_str("unix"));
-        qdict_put(output_options, "server.data.path", qstring_from_str(path));
+        qdict_put(output_options, "server.path", qstring_from_str(path));
     } else if (host) {
         qdict_put(output_options, "server.type", qstring_from_str("inet"));
-        qdict_put(output_options, "server.data.host", qstring_from_str(host));
-        qdict_put(output_options, "server.data.port",
+        qdict_put(output_options, "server.host", qstring_from_str(host));
+        qdict_put(output_options, "server.port",
                   qstring_from_str(port ?: stringify(NBD_DEFAULT_PORT)));
     }
 
     return true;
 }
 
-static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, Error **errp)
+static SocketAddressFlat *nbd_config(BDRVNBDState *s, QDict *options,
+                                     Error **errp)
 {
-    SocketAddress *saddr = NULL;
+    SocketAddressFlat *saddr = NULL;
     QDict *addr = NULL;
     QObject *crumpled_addr = NULL;
     Visitor *iv = NULL;
@@ -287,7 +327,7 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, Error **errp)
      * visitor expects the former.
      */
     iv = qobject_input_visitor_new(crumpled_addr);
-    visit_type_SocketAddress(iv, NULL, &saddr, &local_err);
+    visit_type_SocketAddressFlat(iv, NULL, &saddr, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto done;
@@ -306,9 +346,10 @@ NBDClientSession *nbd_get_client_session(BlockDriverState *bs)
     return &s->client;
 }
 
-static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
+static QIOChannelSocket *nbd_establish_connection(SocketAddressFlat *saddr_flat,
                                                   Error **errp)
 {
+    SocketAddress *saddr = socket_address_crumple(saddr_flat);
     QIOChannelSocket *sioc;
     Error *local_err = NULL;
 
@@ -318,6 +359,7 @@ static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
     qio_channel_socket_connect_sync(sioc,
                                     saddr,
                                     &local_err);
+    qapi_free_SocketAddress(saddr);
     if (local_err) {
         error_propagate(errp, local_err);
         return NULL;
@@ -409,7 +451,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto error;
     }
 
-    /* Translate @host, @port, and @path to a SocketAddress */
+    /* Translate @host, @port, and @path to a SocketAddressFlat */
     if (!nbd_process_legacy_socket_options(options, opts, errp)) {
         goto error;
     }
@@ -430,11 +472,11 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
         }
 
         /* TODO SOCKET_ADDRESS_KIND_FD where fd has AF_INET or AF_INET6 */
-        if (s->saddr->type != SOCKET_ADDRESS_KIND_INET) {
+        if (s->saddr->type != SOCKET_ADDRESS_FLAT_TYPE_INET) {
             error_setg(errp, "TLS only supported over IP sockets");
             goto error;
         }
-        hostname = s->saddr->u.inet.data->host;
+        hostname = s->saddr->u.inet.host;
     }
 
     /* establish TCP connection, return error if it fails
@@ -457,7 +499,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
         object_unref(OBJECT(tlscreds));
     }
     if (ret < 0) {
-        qapi_free_SocketAddress(s->saddr);
+        qapi_free_SocketAddressFlat(s->saddr);
         g_free(s->export);
         g_free(s->tlscredsid);
     }
@@ -483,7 +525,7 @@ static void nbd_close(BlockDriverState *bs)
 
     nbd_client_close(bs);
 
-    qapi_free_SocketAddress(s->saddr);
+    qapi_free_SocketAddressFlat(s->saddr);
     g_free(s->export);
     g_free(s->tlscredsid);
 }
@@ -514,15 +556,15 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
     Visitor *ov;
     const char *host = NULL, *port = NULL, *path = NULL;
 
-    if (s->saddr->type == SOCKET_ADDRESS_KIND_INET) {
-        const InetSocketAddress *inet = s->saddr->u.inet.data;
+    if (s->saddr->type == SOCKET_ADDRESS_FLAT_TYPE_INET) {
+        const InetSocketAddress *inet = &s->saddr->u.inet;
         if (!inet->has_ipv4 && !inet->has_ipv6 && !inet->has_to) {
             host = inet->host;
             port = inet->port;
         }
-    } else if (s->saddr->type == SOCKET_ADDRESS_KIND_UNIX) {
-        path = s->saddr->u.q_unix.data->path;
-    }
+    } else if (s->saddr->type == SOCKET_ADDRESS_FLAT_TYPE_UNIX) {
+        path = s->saddr->u.q_unix.path;
+    } /* else can't represent as pseudo-filename */
 
     qdict_put(opts, "driver", qstring_from_str("nbd"));
 
@@ -541,7 +583,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
     }
 
     ov = qobject_output_visitor_new(&saddr_qdict);
-    visit_type_SocketAddress(ov, NULL, &s->saddr, &error_abort);
+    visit_type_SocketAddressFlat(ov, NULL, &s->saddr, &error_abort);
     visit_complete(ov, &saddr_qdict);
     visit_free(ov);
     qdict_put_obj(opts, "server", saddr_qdict);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4e8e4e3..8d87962 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2762,7 +2762,7 @@
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsNbd',
-  'data': { 'server': 'SocketAddress',
+  'data': { 'server': 'SocketAddressFlat',
             '*export': 'str',
             '*tls-creds': 'str' } }
 
-- 
2.7.4

Re: [Qemu-devel] [RFC v2 for-2.9 08/10] nbd: Tidy up blockdev-add interface

[Eric Blake]

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

On 03/30/2017 08:15 AM, Markus Armbruster wrote:
> SocketAddress is a simple union, and simple unions are awkward: they
> have their variant members wrapped in a "data" object on the wire, and
> require additional indirections in C.  I intend to limit its use to
> existing external interfaces, and convert all internal interfaces to
> SocketAddressFlat.
> 
> BlockdevOptionsNbd is an external interface using SocketAddress.  We
> already use SocketAddressFlat elsewhere in blockdev=add.  Replace it
> by SocketAddressFlat while we can (it's new in 2.9) for simplicity and
> consistency.  For example,
> 
>     { "execute": "blockdev-add",
>       "arguments": { "node-name": "foo", "driver": "nbd",
>                      "server": { "type": "inet",
> 		                 "data": { "host": "localhost",
> 				           "port": "12345" } } } }
> 
> becomes
> 
>     { "execute": "blockdev-add",
>       "arguments": { "node-name": "foo", "driver": "nbd",
>                      "server": { "type": "inet",
> 		                 "host": "localhost", "port": "12345" } } }
> 
> Since the internal interfaces still take SocketAddress, this requires
> conversion function socket_address_crumple().  It'll go away when I
> update the interfaces.
> 

So far, so good.

> Unfortunately, SocketAddress is also visible in -drive since 2.8.  Add
> still more gunk to nbd_process_legacy_socket_options().  You can now
> shorten

Dead commit message comment?  Or did you still leave it in here, with
one last chance to approve it before ripping it out in v3 for comparison?

/me reads patch - looks like you did not remove the gunk yet on this
revision

> 
>     -drive if=none,driver=nbd,server.type=inet,server.data.host=127.0.0.1,server.data.port=12345
> 
> to
> 
>     -drive if=none,driver=nbd,server.type=inet,server.host=127.0.0.1,server.port=12345

The example is useful, but if you drop the compatibility gunk, you'll
want to reword this as an intentional change in (heretofore
undocumented) behavior.

I'm ambivalent on whether to keep or remove the server.* gunk (the
conservative approach is to keep it for at least 2.9, even if we rip it
out in 2.10, as we already found out with qom-type that not being
conservative during hard freeze risks unintentional breakage - but the
counter-argument is that -drive parameters have been undocumented and
that libvirt is not using 2.8's server.data, so it is unlikely anyone
else is either).  Or put another way, we've already taken care of
back-compat to make sure that the legacy 'host' works, whether or not
the new form is spelled 'server.host' or 'server.data.host', so breaking
back-compat with 'server.data.host' should not impact clients that are
only using the older 'host'.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/nbd.c          | 94 +++++++++++++++++++++++++++++++++++++---------------
>  qapi/block-core.json |  2 +-
>  2 files changed, 69 insertions(+), 27 deletions(-)
> 

> @@ -223,12 +223,51 @@ static bool nbd_process_legacy_socket_options(QDict *output_options,
>      const char *path = qemu_opt_get(legacy_opts, "path");
>      const char *host = qemu_opt_get(legacy_opts, "host");
>      const char *port = qemu_opt_get(legacy_opts, "port");
> +    const char *sd_path = qdict_get_try_str(output_options,
> +                                            "server.data.path");
> +    const char *sd_host = qdict_get_try_str(output_options,
> +                                            "server.data.host");
> +    const char *sd_port = qdict_get_try_str(output_options,
> +                                            "server.data.port");
> +    bool bare = path || host || port;
> +    bool server_data = sd_path || sd_host || sd_port;

Ah, so diff from v1 is that you took my suggestion of 'bare' for making
the compatibility gunk checks easier to read.


> +    if (bare && server_data) {
> +        error_setg(errp, "Cannot use 'server' and path/host/port at the "
> +                   "same time");
> +        return false;
> +    }
> +
> +    if (server_data) {
> +        if (sd_host) {
> +            val = qdict_get(output_options, "server.data.host");
> +            qobject_incref(val);
> +            qdict_put_obj(output_options, "server.host", val);
> +            qdict_del(output_options, "server.data.host");
> +        }
> +        if (sd_port) {
> +            val = qdict_get(output_options, "server.data.port");
> +            qobject_incref(val);
> +            qdict_put_obj(output_options, "server.port", val);
> +            qdict_del(output_options, "server.data.port");
> +        }
> +        if (sd_path) {
> +            val = qdict_get(output_options, "server.data.path");
> +            qobject_incref(val);
> +            qdict_put_obj(output_options, "server.path", val);
> +            qdict_del(output_options, "server.data.path");
> +        }
> +        return true;

This exits early, possibly setting both server.host and server.path, and
misses out on the fact that 'bare' mode flags:

    if (path && host) {
        error_setg(errp, "path and host may not be used at the same time");
        return false;

If I'm understanding it right, this will still be flagged later during
the QAPI type visit as invalid, although the error message quality may
be different.

Would it be any better to just use the same validation logic for both,
by instead writing:

if (server_data) {
    if (sd_host) {
        host = sd_host;
    }
    if (sd_port) {
        port = sd_port;
    }
    if (sd_path) {
        path = sd_path;
    }
} else {

> +    }
> +
> +    assert(bare);
> +
>      for (e = qdict_first(output_options); e; e = qdict_next(output_options, e))
>      {
>          if (strstart(e->key, "server.", NULL)) {

to enclose this for loop to only happen on the bare branch, having both
branches then fall into the validation code?

> @@ -248,20 +287,21 @@ static bool nbd_process_legacy_socket_options(QDict *output_options,
>          }
>  
>          qdict_put(output_options, "server.type", qstring_from_str("unix"));
> -        qdict_put(output_options, "server.data.path", qstring_from_str(path));
> +        qdict_put(output_options, "server.path", qstring_from_str(path));
>      } else if (host) {
>          qdict_put(output_options, "server.type", qstring_from_str("inet"));
> -        qdict_put(output_options, "server.data.host", qstring_from_str(host));
> -        qdict_put(output_options, "server.data.port",
> +        qdict_put(output_options, "server.host", qstring_from_str(host));
> +        qdict_put(output_options, "server.port",
>                    qstring_from_str(port ?: stringify(NBD_DEFAULT_PORT)));

Perhaps another reason to fall through: server.port is mandatory in the
schema (for the 'inet' branch of the union), but 'port' is optional on
the command line by populating the default here.  Returning early on the
server_data form makes 'server.data.port' mandatory, if you use the
server.* form; I guess it's a question of whether we want the server.*
form to match the bare form, or whether it can be as strict as the QMP
form and only the bare form has to have compatibility gunk.  Or maybe,
as argued by others, we don't want 'server.data'*' back-compat at all.


-- 
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] [RFC v2 for-2.9 08/10] nbd: Tidy up blockdev-add interface

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/30/2017 08:15 AM, Markus Armbruster wrote:
>> SocketAddress is a simple union, and simple unions are awkward: they
>> have their variant members wrapped in a "data" object on the wire, and
>> require additional indirections in C.  I intend to limit its use to
>> existing external interfaces, and convert all internal interfaces to
>> SocketAddressFlat.
>> 
>> BlockdevOptionsNbd is an external interface using SocketAddress.  We
>> already use SocketAddressFlat elsewhere in blockdev=add.  Replace it
>> by SocketAddressFlat while we can (it's new in 2.9) for simplicity and
>> consistency.  For example,
>> 
>>     { "execute": "blockdev-add",
>>       "arguments": { "node-name": "foo", "driver": "nbd",
>>                      "server": { "type": "inet",
>> 		                 "data": { "host": "localhost",
>> 				           "port": "12345" } } } }
>> 
>> becomes
>> 
>>     { "execute": "blockdev-add",
>>       "arguments": { "node-name": "foo", "driver": "nbd",
>>                      "server": { "type": "inet",
>> 		                 "host": "localhost", "port": "12345" } } }
>> 
>> Since the internal interfaces still take SocketAddress, this requires
>> conversion function socket_address_crumple().  It'll go away when I
>> update the interfaces.
>> 
>
> So far, so good.
>
>> Unfortunately, SocketAddress is also visible in -drive since 2.8.  Add
>> still more gunk to nbd_process_legacy_socket_options().  You can now
>> shorten
>
> Dead commit message comment?  Or did you still leave it in here, with
> one last chance to approve it before ripping it out in v3 for comparison?
>
> /me reads patch - looks like you did not remove the gunk yet on this
> revision
>
>> 
>>     -drive if=none,driver=nbd,server.type=inet,server.data.host=127.0.0.1,server.data.port=12345
>> 
>> to
>> 
>>     -drive if=none,driver=nbd,server.type=inet,server.host=127.0.0.1,server.port=12345
>
> The example is useful, but if you drop the compatibility gunk, you'll
> want to reword this as an intentional change in (heretofore
> undocumented) behavior.
>
> I'm ambivalent on whether to keep or remove the server.* gunk (the
> conservative approach is to keep it for at least 2.9, even if we rip it
> out in 2.10, as we already found out with qom-type that not being
> conservative during hard freeze risks unintentional breakage - but the
> counter-argument is that -drive parameters have been undocumented and
> that libvirt is not using 2.8's server.data, so it is unlikely anyone
> else is either).  Or put another way, we've already taken care of
> back-compat to make sure that the legacy 'host' works, whether or not
> the new form is spelled 'server.host' or 'server.data.host', so breaking
> back-compat with 'server.data.host' should not impact clients that are
> only using the older 'host'.

Yes.

I'd like to use the license to break this undocumented interface offered
Paolo et al, but I feel offering everybody one more opportunity to
object can't hurt.

>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/nbd.c          | 94 +++++++++++++++++++++++++++++++++++++---------------
>>  qapi/block-core.json |  2 +-
>>  2 files changed, 69 insertions(+), 27 deletions(-)
>> 
>
>> @@ -223,12 +223,51 @@ static bool nbd_process_legacy_socket_options(QDict *output_options,
>>      const char *path = qemu_opt_get(legacy_opts, "path");
>>      const char *host = qemu_opt_get(legacy_opts, "host");
>>      const char *port = qemu_opt_get(legacy_opts, "port");
>> +    const char *sd_path = qdict_get_try_str(output_options,
>> +                                            "server.data.path");
>> +    const char *sd_host = qdict_get_try_str(output_options,
>> +                                            "server.data.host");
>> +    const char *sd_port = qdict_get_try_str(output_options,
>> +                                            "server.data.port");
>> +    bool bare = path || host || port;
>> +    bool server_data = sd_path || sd_host || sd_port;
>
> Ah, so diff from v1 is that you took my suggestion of 'bare' for making
> the compatibility gunk checks easier to read.
>
>
>> +    if (bare && server_data) {
>> +        error_setg(errp, "Cannot use 'server' and path/host/port at the "
>> +                   "same time");
>> +        return false;
>> +    }
>> +
>> +    if (server_data) {
>> +        if (sd_host) {
>> +            val = qdict_get(output_options, "server.data.host");
>> +            qobject_incref(val);
>> +            qdict_put_obj(output_options, "server.host", val);
>> +            qdict_del(output_options, "server.data.host");
>> +        }
>> +        if (sd_port) {
>> +            val = qdict_get(output_options, "server.data.port");
>> +            qobject_incref(val);
>> +            qdict_put_obj(output_options, "server.port", val);
>> +            qdict_del(output_options, "server.data.port");
>> +        }
>> +        if (sd_path) {
>> +            val = qdict_get(output_options, "server.data.path");
>> +            qobject_incref(val);
>> +            qdict_put_obj(output_options, "server.path", val);
>> +            qdict_del(output_options, "server.data.path");
>> +        }
>> +        return true;
>
> This exits early, possibly setting both server.host and server.path, and
> misses out on the fact that 'bare' mode flags:
>
>     if (path && host) {
>         error_setg(errp, "path and host may not be used at the same time");
>         return false;
>
> If I'm understanding it right, this will still be flagged later during
> the QAPI type visit as invalid, although the error message quality may
> be different.

s/different/not so hot/

> Would it be any better to just use the same validation logic for both,
> by instead writing:
>
> if (server_data) {
>     if (sd_host) {
>         host = sd_host;
>     }
>     if (sd_port) {
>         port = sd_port;
>     }
>     if (sd_path) {
>         path = sd_path;
>     }
> } else {
>
>> +    }
>> +
>> +    assert(bare);
>> +
>>      for (e = qdict_first(output_options); e; e = qdict_next(output_options, e))
>>      {
>>          if (strstart(e->key, "server.", NULL)) {
>
> to enclose this for loop to only happen on the bare branch, having both
> branches then fall into the validation code?

My reason for doing it this way is to avoid changing the existing gunk
as much as I possibly can, even when that leads to suboptimal error
messages.

>> @@ -248,20 +287,21 @@ static bool nbd_process_legacy_socket_options(QDict *output_options,
>>          }
>>  
>>          qdict_put(output_options, "server.type", qstring_from_str("unix"));
>> -        qdict_put(output_options, "server.data.path", qstring_from_str(path));
>> +        qdict_put(output_options, "server.path", qstring_from_str(path));
>>      } else if (host) {
>>          qdict_put(output_options, "server.type", qstring_from_str("inet"));
>> -        qdict_put(output_options, "server.data.host", qstring_from_str(host));
>> -        qdict_put(output_options, "server.data.port",
>> +        qdict_put(output_options, "server.host", qstring_from_str(host));
>> +        qdict_put(output_options, "server.port",
>>                    qstring_from_str(port ?: stringify(NBD_DEFAULT_PORT)));
>
> Perhaps another reason to fall through: server.port is mandatory in the
> schema (for the 'inet' branch of the union), but 'port' is optional on
> the command line by populating the default here.  Returning early on the
> server_data form makes 'server.data.port' mandatory, if you use the
> server.* form; I guess it's a question of whether we want the server.*
> form to match the bare form, or whether it can be as strict as the QMP
> form and only the bare form has to have compatibility gunk.  Or maybe,
> as argued by others, we don't want 'server.data'*' back-compat at all.

If we decide to keep the new compatibility gunk (and I hope we won't),
we can improve it in 2.10.

Re: [Qemu-devel] [RFC v2 for-2.9 08/10] nbd: Tidy up blockdev-add interface

[Max Reitz]

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

On 30.03.2017 15:15, Markus Armbruster wrote:
> SocketAddress is a simple union, and simple unions are awkward: they
> have their variant members wrapped in a "data" object on the wire, and
> require additional indirections in C.  I intend to limit its use to
> existing external interfaces, and convert all internal interfaces to
> SocketAddressFlat.
> 
> BlockdevOptionsNbd is an external interface using SocketAddress.  We
> already use SocketAddressFlat elsewhere in blockdev=add.  Replace it

s/=/-/

> by SocketAddressFlat while we can (it's new in 2.9) for simplicity and
> consistency.  For example,
> 
>     { "execute": "blockdev-add",
>       "arguments": { "node-name": "foo", "driver": "nbd",
>                      "server": { "type": "inet",
> 		                 "data": { "host": "localhost",
> 				           "port": "12345" } } } }
> 
> becomes
> 
>     { "execute": "blockdev-add",
>       "arguments": { "node-name": "foo", "driver": "nbd",
>                      "server": { "type": "inet",
> 		                 "host": "localhost", "port": "12345" } } }
> 
> Since the internal interfaces still take SocketAddress, this requires
> conversion function socket_address_crumple().  It'll go away when I
> update the interfaces.
> 
> Unfortunately, SocketAddress is also visible in -drive since 2.8.  Add
> still more gunk to nbd_process_legacy_socket_options().  You can now
> shorten
> 
>     -drive if=none,driver=nbd,server.type=inet,server.data.host=127.0.0.1,server.data.port=12345
> 
> to
> 
>     -drive if=none,driver=nbd,server.type=inet,server.host=127.0.0.1,server.port=12345
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/nbd.c          | 94 +++++++++++++++++++++++++++++++++++++---------------
>  qapi/block-core.json |  2 +-
>  2 files changed, 69 insertions(+), 27 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

(Hoping the next patch gets squashed in)


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

[Qemu-devel] [RFC v2 for-2.9 09/10] squash! nbd: Tidy up blockdev-add interface

[Markus Armbruster]

Drop backward -drive server.data.* compatibility gunk.  On squash,
replace commit message's last paragraph "Unfortunately, SocketAddress
is also visible..." by:

Unfortunately, SocketAddress is also visible in -drive since 2.8:

    -drive if=none,driver=nbd,server.type=inet,server.data.host=127.0.0.1,server.data.port=12345

Nobody should be using it, as it's fairly new and has never been
documented, so adding still more compatibility gunk to keep it working
isn't worth the trouble.  You now have to use

    -drive if=none,driver=nbd,server.type=inet,server.host=127.0.0.1,server.port=12345

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/nbd.c | 41 +----------------------------------------
 1 file changed, 1 insertion(+), 40 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index ea9d8dc..8bb29a9 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -223,51 +223,12 @@ static bool nbd_process_legacy_socket_options(QDict *output_options,
     const char *path = qemu_opt_get(legacy_opts, "path");
     const char *host = qemu_opt_get(legacy_opts, "host");
     const char *port = qemu_opt_get(legacy_opts, "port");
-    const char *sd_path = qdict_get_try_str(output_options,
-                                            "server.data.path");
-    const char *sd_host = qdict_get_try_str(output_options,
-                                            "server.data.host");
-    const char *sd_port = qdict_get_try_str(output_options,
-                                            "server.data.port");
-    bool bare = path || host || port;
-    bool server_data = sd_path || sd_host || sd_port;
-    QObject *val;
     const QDictEntry *e;
 
-    if (!bare && !server_data) {
+    if (!path && !host && !port) {
         return true;
     }
 
-    if (bare && server_data) {
-        error_setg(errp, "Cannot use 'server' and path/host/port at the "
-                   "same time");
-        return false;
-    }
-
-    if (server_data) {
-        if (sd_host) {
-            val = qdict_get(output_options, "server.data.host");
-            qobject_incref(val);
-            qdict_put_obj(output_options, "server.host", val);
-            qdict_del(output_options, "server.data.host");
-        }
-        if (sd_port) {
-            val = qdict_get(output_options, "server.data.port");
-            qobject_incref(val);
-            qdict_put_obj(output_options, "server.port", val);
-            qdict_del(output_options, "server.data.port");
-        }
-        if (sd_path) {
-            val = qdict_get(output_options, "server.data.path");
-            qobject_incref(val);
-            qdict_put_obj(output_options, "server.path", val);
-            qdict_del(output_options, "server.data.path");
-        }
-        return true;
-    }
-
-    assert(bare);
-
     for (e = qdict_first(output_options); e; e = qdict_next(output_options, e))
     {
         if (strstart(e->key, "server.", NULL)) {
-- 
2.7.4

Re: [Qemu-devel] [RFC v2 for-2.9 09/10] squash! nbd: Tidy up blockdev-add interface

[Eric Blake]

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

On 03/30/2017 08:15 AM, Markus Armbruster wrote:
> Drop backward -drive server.data.* compatibility gunk.  On squash,
> replace commit message's last paragraph "Unfortunately, SocketAddress
> is also visible..." by:

Maybe I should glance at the whole thread before reviewing without
realizing what is coming later ;)

> 
> Unfortunately, SocketAddress is also visible in -drive since 2.8:
> 
>     -drive if=none,driver=nbd,server.type=inet,server.data.host=127.0.0.1,server.data.port=12345
> 
> Nobody should be using it, as it's fairly new and has never been
> documented, so adding still more compatibility gunk to keep it working
> isn't worth the trouble.  You now have to use
> 
>     -drive if=none,driver=nbd,server.type=inet,server.host=127.0.0.1,server.port=12345

As pointed out on 8/10, it might be worth documenting that while legacy
'port' is optional, the new 'server.port' is mandatory.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/nbd.c | 41 +----------------------------------------
>  1 file changed, 1 insertion(+), 40 deletions(-)
> 

When squashed with 8/10, it does make that patch more palatable (all the
concerns I expressed there were ripped out here).  So if no one else has
a strong argument for keeping 'server.data.*' back-compat, then the
squash of 8+9 together can have:

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] [RFC v2 for-2.9 09/10] squash! nbd: Tidy up blockdev-add interface

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/30/2017 08:15 AM, Markus Armbruster wrote:
>> Drop backward -drive server.data.* compatibility gunk.  On squash,
>> replace commit message's last paragraph "Unfortunately, SocketAddress
>> is also visible..." by:
>
> Maybe I should glance at the whole thread before reviewing without
> realizing what is coming later ;)

Maybe I should point out such stunts in my cover letters :)

>> Unfortunately, SocketAddress is also visible in -drive since 2.8:
>> 
>>     -drive if=none,driver=nbd,server.type=inet,server.data.host=127.0.0.1,server.data.port=12345
>> 
>> Nobody should be using it, as it's fairly new and has never been
>> documented, so adding still more compatibility gunk to keep it working
>> isn't worth the trouble.  You now have to use
>> 
>>     -drive if=none,driver=nbd,server.type=inet,server.host=127.0.0.1,server.port=12345
>
> As pointed out on 8/10, it might be worth documenting that while legacy
> 'port' is optional, the new 'server.port' is mandatory.

The combined commit message doesn't mention the "bare" legacy syntax.
I'd have to explain it first.  I'm afraid it would be a distraction.
server.port is mandatory elsewhere, too.

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/nbd.c | 41 +----------------------------------------
>>  1 file changed, 1 insertion(+), 40 deletions(-)
>> 
>
> When squashed with 8/10, it does make that patch more palatable (all the
> concerns I expressed there were ripped out here).  So if no one else has
> a strong argument for keeping 'server.data.*' back-compat, then the
> squash of 8+9 together can have:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [RFC v2 for-2.9 09/10] squash! nbd: Tidy up blockdev-add interface

[Max Reitz]

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

On 30.03.2017 15:15, Markus Armbruster wrote:
> Drop backward -drive server.data.* compatibility gunk.  On squash,
> replace commit message's last paragraph "Unfortunately, SocketAddress
> is also visible..." by:
> 
> Unfortunately, SocketAddress is also visible in -drive since 2.8:
> 
>     -drive if=none,driver=nbd,server.type=inet,server.data.host=127.0.0.1,server.data.port=12345
> 
> Nobody should be using it, as it's fairly new and has never been
> documented, so adding still more compatibility gunk to keep it working
> isn't worth the trouble.  You now have to use
> 
>     -drive if=none,driver=nbd,server.type=inet,server.host=127.0.0.1,server.port=12345
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/nbd.c | 41 +----------------------------------------
>  1 file changed, 1 insertion(+), 40 deletions(-)

(Very-happily-)Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [RFC v2 for-2.9 10/10] sheepdog: Fix blockdev-add

[Markus Armbruster]

Commit 831acdc "sheepdog: Implement bdrv_parse_filename()" and commit
d282f34 "sheepdog: Support blockdev-add" have different ideas on how
the QemuOpts parameters for the server address are named.  Fix that.
While there, rename BlockdevOptionsSheepdog member addr to server, for
consistency with BlockdevOptionsSsh, BlockdevOptionsGluster,
BlockdevOptionsNbd.

Commit 831acdc's example becomes

    --drive driver=sheepdog,server.type=inet,server.host=fido,server.port=7000,vdi=dolly

instead of

    --drive driver=sheepdog,host=fido,vdi=dolly

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c     | 84 ++++++++++++++++++++++++++++++++++------------------
 qapi/block-core.json |  4 +--
 2 files changed, 58 insertions(+), 30 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 89e98ed..c81013d 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -13,9 +13,11 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi-visit.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qint.h"
+#include "qapi/qobject-input-visitor.h"
 #include "qemu/uri.h"
 #include "qemu/error-report.h"
 #include "qemu/sockets.h"
@@ -547,6 +549,47 @@ static SocketAddress *sd_socket_address(const char *path,
     return addr;
 }
 
+static SocketAddress *sd_server_config(QDict *options, Error **errp)
+{
+    QDict *server = NULL;
+    QObject *crumpled_server = NULL;
+    Visitor *iv = NULL;
+    SocketAddressFlat *saddr_flat = NULL;
+    SocketAddress *saddr = NULL;
+    Error *local_err = NULL;
+
+    qdict_extract_subqdict(options, &server, "server.");
+
+    crumpled_server = qdict_crumple(server, errp);
+    if (!crumpled_server) {
+        goto done;
+    }
+
+    /*
+     * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive
+     * server.type=inet.  .to doesn't matter, it's ignored anyway.
+     * That's because when @options come from -blockdev or
+     * blockdev_add, members are typed according to the QAPI schema,
+     * but when they come from -drive, they're all QString.  The
+     * visitor expects the former.
+     */
+    iv = qobject_input_visitor_new(crumpled_server);
+    visit_type_SocketAddressFlat(iv, NULL, &saddr_flat, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto done;
+    }
+
+    saddr = socket_address_crumple(saddr_flat);
+
+done:
+    qapi_free_SocketAddressFlat(saddr_flat);
+    visit_free(iv);
+    qobject_decref(crumpled_server);
+    QDECREF(server);
+    return saddr;
+}
+
 /* Return -EIO in case of error, file descriptor on success */
 static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
 {
@@ -1175,15 +1218,17 @@ static void sd_parse_filename(const char *filename, QDict *options,
     }
 
     if (cfg.host) {
-        qdict_set_default_str(options, "host", cfg.host);
+        qdict_set_default_str(options, "server.host", cfg.host);
+        qdict_set_default_str(options, "server.type", "inet");
     }
     if (cfg.port) {
         snprintf(buf, sizeof(buf), "%d", cfg.port);
-        qdict_set_default_str(options, "port", buf);
+        qdict_set_default_str(options, "server.port", buf);
     }
     if (cfg.path) {
-        qdict_set_default_str(options, "path", cfg.path);
-    }
+        qdict_set_default_str(options, "server.path", cfg.path); 
+        qdict_set_default_str(options, "server.type", "unix");
+   }
     qdict_set_default_str(options, "vdi", cfg.vdi);
     qdict_set_default_str(options, "tag", cfg.tag);
     if (cfg.snap_id) {
@@ -1510,18 +1555,6 @@ static QemuOptsList runtime_opts = {
     .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
     .desc = {
         {
-            .name = "host",
-            .type = QEMU_OPT_STRING,
-        },
-        {
-            .name = "port",
-            .type = QEMU_OPT_STRING,
-        },
-        {
-            .name = "path",
-            .type = QEMU_OPT_STRING,
-        },
-        {
             .name = "vdi",
             .type = QEMU_OPT_STRING,
         },
@@ -1543,7 +1576,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     int ret, fd;
     uint32_t vid = 0;
     BDRVSheepdogState *s = bs->opaque;
-    const char *host, *port, *path, *vdi, *snap_id_str, *tag;
+    const char *vdi, *snap_id_str, *tag;
     uint64_t snap_id;
     char *buf = NULL;
     QemuOpts *opts;
@@ -1560,20 +1593,17 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
         goto err_no_fd;
     }
 
-    host = qemu_opt_get(opts, "host");
-    port = qemu_opt_get(opts, "port");
-    path = qemu_opt_get(opts, "path");
+    s->addr = sd_server_config(options, errp);
+    if (!s->addr) {
+        ret = -EINVAL;
+        goto err_no_fd;
+    }
+        
     vdi = qemu_opt_get(opts, "vdi");
     snap_id_str = qemu_opt_get(opts, "snap-id");
     snap_id = qemu_opt_get_number(opts, "snap-id", CURRENT_VDI_ID);
     tag = qemu_opt_get(opts, "tag");
 
-    if ((host || port) && path) {
-        error_setg(errp, "can't use 'path' together with 'host' or 'port'");
-        ret = -EINVAL;
-        goto err_no_fd;
-    }
-
     if (!vdi) {
         error_setg(errp, "parameter 'vdi' is missing");
         ret = -EINVAL;
@@ -1604,8 +1634,6 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
         goto err_no_fd;
     }
 
-    s->addr = sd_socket_address(path, host, port);
-
     QLIST_INIT(&s->inflight_aio_head);
     QLIST_INIT(&s->failed_aio_head);
     QLIST_INIT(&s->inflight_aiocb_head);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8d87962..b5f0e99 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2623,7 +2623,7 @@
 # Driver specific block device options for sheepdog
 #
 # @vdi:         Virtual disk image name
-# @addr:        The Sheepdog server to connect to
+# @server:      The Sheepdog server to connect to
 # @snap-id:     Snapshot ID
 # @tag:         Snapshot tag name
 #
@@ -2632,7 +2632,7 @@
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsSheepdog',
-  'data': { 'addr': 'SocketAddressFlat',
+  'data': { 'server': 'SocketAddressFlat',
             'vdi': 'str',
             '*snap-id': 'uint32',
             '*tag': 'str' } }
-- 
2.7.4

Re: [Qemu-devel] [RFC v2 for-2.9 10/10] sheepdog: Fix blockdev-add

[Eric Blake]

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

On 03/30/2017 08:15 AM, Markus Armbruster wrote:
> Commit 831acdc "sheepdog: Implement bdrv_parse_filename()" and commit
> d282f34 "sheepdog: Support blockdev-add" have different ideas on how
> the QemuOpts parameters for the server address are named.  Fix that.
> While there, rename BlockdevOptionsSheepdog member addr to server, for
> consistency with BlockdevOptionsSsh, BlockdevOptionsGluster,
> BlockdevOptionsNbd.
> 
> Commit 831acdc's example becomes
> 
>     --drive driver=sheepdog,server.type=inet,server.host=fido,server.port=7000,vdi=dolly
> 
> instead of
> 
>     --drive driver=sheepdog,host=fido,vdi=dolly
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c     | 84 ++++++++++++++++++++++++++++++++++------------------
>  qapi/block-core.json |  4 +--
>  2 files changed, 58 insertions(+), 30 deletions(-)
> 

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] [RFC v2 for-2.9 10/10] sheepdog: Fix blockdev-add

[Max Reitz]

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

On 30.03.2017 15:15, Markus Armbruster wrote:
> Commit 831acdc "sheepdog: Implement bdrv_parse_filename()" and commit
> d282f34 "sheepdog: Support blockdev-add" have different ideas on how
> the QemuOpts parameters for the server address are named.  Fix that.
> While there, rename BlockdevOptionsSheepdog member addr to server, for
> consistency with BlockdevOptionsSsh, BlockdevOptionsGluster,
> BlockdevOptionsNbd.
> 
> Commit 831acdc's example becomes
> 
>     --drive driver=sheepdog,server.type=inet,server.host=fido,server.port=7000,vdi=dolly
> 
> instead of
> 
>     --drive driver=sheepdog,host=fido,vdi=dolly
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c     | 84 ++++++++++++++++++++++++++++++++++------------------
>  qapi/block-core.json |  4 +--
>  2 files changed, 58 insertions(+), 30 deletions(-)

There's trailing whitespace somewhere in this patch (git-am complained).

> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 89e98ed..c81013d 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -13,9 +13,11 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qapi-visit.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qint.h"
> +#include "qapi/qobject-input-visitor.h"
>  #include "qemu/uri.h"
>  #include "qemu/error-report.h"
>  #include "qemu/sockets.h"
> @@ -547,6 +549,47 @@ static SocketAddress *sd_socket_address(const char *path,
>      return addr;
>  }
>  
> +static SocketAddress *sd_server_config(QDict *options, Error **errp)
> +{
> +    QDict *server = NULL;
> +    QObject *crumpled_server = NULL;
> +    Visitor *iv = NULL;
> +    SocketAddressFlat *saddr_flat = NULL;
> +    SocketAddress *saddr = NULL;
> +    Error *local_err = NULL;
> +
> +    qdict_extract_subqdict(options, &server, "server.");

server may be empty:

$ ./qemu-img info sheepdog:vdi
qemu-img: Could not open 'sheepdog:vdi': Parameter 'type' is missing

I'd propose creating an 'inet' SocketAddress object then for
SD_DEFAULT_ADDR and SD_DEFAULT_PORT so that existing behavior won't change.

> +
> +    crumpled_server = qdict_crumple(server, errp);
> +    if (!crumpled_server) {
> +        goto done;
> +    }
> +
> +    /*
> +     * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive
> +     * server.type=inet.  .to doesn't matter, it's ignored anyway.
> +     * That's because when @options come from -blockdev or
> +     * blockdev_add, members are typed according to the QAPI schema,
> +     * but when they come from -drive, they're all QString.  The
> +     * visitor expects the former.
> +     */
> +    iv = qobject_input_visitor_new(crumpled_server);
> +    visit_type_SocketAddressFlat(iv, NULL, &saddr_flat, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto done;
> +    }
> +
> +    saddr = socket_address_crumple(saddr_flat);
> +
> +done:
> +    qapi_free_SocketAddressFlat(saddr_flat);
> +    visit_free(iv);
> +    qobject_decref(crumpled_server);
> +    QDECREF(server);
> +    return saddr;
> +}
> +
>  /* Return -EIO in case of error, file descriptor on success */
>  static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
>  {
> @@ -1175,15 +1218,17 @@ static void sd_parse_filename(const char *filename, QDict *options,
>      }
>  
>      if (cfg.host) {
> -        qdict_set_default_str(options, "host", cfg.host);
> +        qdict_set_default_str(options, "server.host", cfg.host);
> +        qdict_set_default_str(options, "server.type", "inet");
>      }
>      if (cfg.port) {
>          snprintf(buf, sizeof(buf), "%d", cfg.port);
> -        qdict_set_default_str(options, "port", buf);
> +        qdict_set_default_str(options, "server.port", buf);

.port is mandatory, so doing this optionally results in:

$ ./qemu-img info sheepdog://localhost/foo
qemu-img: Could not open 'sheepdog://localhost/foo': Parameter 'port' is
missing

>      }
>      if (cfg.path) {
> -        qdict_set_default_str(options, "path", cfg.path);
> -    }
> +        qdict_set_default_str(options, "server.path", cfg.path); 
> +        qdict_set_default_str(options, "server.type", "unix");
> +   }

Indentation is off.

Max

>      qdict_set_default_str(options, "vdi", cfg.vdi);
>      qdict_set_default_str(options, "tag", cfg.tag);
>      if (cfg.snap_id) {
> @@ -1510,18 +1555,6 @@ static QemuOptsList runtime_opts = {
>      .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
>      .desc = {
>          {
> -            .name = "host",
> -            .type = QEMU_OPT_STRING,
> -        },
> -        {
> -            .name = "port",
> -            .type = QEMU_OPT_STRING,
> -        },
> -        {
> -            .name = "path",
> -            .type = QEMU_OPT_STRING,
> -        },
> -        {
>              .name = "vdi",
>              .type = QEMU_OPT_STRING,
>          },
> @@ -1543,7 +1576,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
>      int ret, fd;
>      uint32_t vid = 0;
>      BDRVSheepdogState *s = bs->opaque;
> -    const char *host, *port, *path, *vdi, *snap_id_str, *tag;
> +    const char *vdi, *snap_id_str, *tag;
>      uint64_t snap_id;
>      char *buf = NULL;
>      QemuOpts *opts;
> @@ -1560,20 +1593,17 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
>          goto err_no_fd;
>      }
>  
> -    host = qemu_opt_get(opts, "host");
> -    port = qemu_opt_get(opts, "port");
> -    path = qemu_opt_get(opts, "path");
> +    s->addr = sd_server_config(options, errp);
> +    if (!s->addr) {
> +        ret = -EINVAL;
> +        goto err_no_fd;
> +    }
> +     
>      vdi = qemu_opt_get(opts, "vdi");
>      snap_id_str = qemu_opt_get(opts, "snap-id");
>      snap_id = qemu_opt_get_number(opts, "snap-id", CURRENT_VDI_ID);
>      tag = qemu_opt_get(opts, "tag");
>  
> -    if ((host || port) && path) {
> -        error_setg(errp, "can't use 'path' together with 'host' or 'port'");
> -        ret = -EINVAL;
> -        goto err_no_fd;
> -    }
> -
>      if (!vdi) {
>          error_setg(errp, "parameter 'vdi' is missing");
>          ret = -EINVAL;
> @@ -1604,8 +1634,6 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
>          goto err_no_fd;
>      }
>  
> -    s->addr = sd_socket_address(path, host, port);
> -
>      QLIST_INIT(&s->inflight_aio_head);
>      QLIST_INIT(&s->failed_aio_head);
>      QLIST_INIT(&s->inflight_aiocb_head);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8d87962..b5f0e99 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2623,7 +2623,7 @@
>  # Driver specific block device options for sheepdog
>  #
>  # @vdi:         Virtual disk image name
> -# @addr:        The Sheepdog server to connect to
> +# @server:      The Sheepdog server to connect to
>  # @snap-id:     Snapshot ID
>  # @tag:         Snapshot tag name
>  #
> @@ -2632,7 +2632,7 @@
>  # Since: 2.9
>  ##
>  { 'struct': 'BlockdevOptionsSheepdog',
> -  'data': { 'addr': 'SocketAddressFlat',
> +  'data': { 'server': 'SocketAddressFlat',
>              'vdi': 'str',
>              '*snap-id': 'uint32',
>              '*tag': 'str' } }
> 



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

Re: [Qemu-devel] [RFC v2 for-2.9 10/10] sheepdog: Fix blockdev-add

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 30.03.2017 15:15, Markus Armbruster wrote:
>> Commit 831acdc "sheepdog: Implement bdrv_parse_filename()" and commit
>> d282f34 "sheepdog: Support blockdev-add" have different ideas on how
>> the QemuOpts parameters for the server address are named.  Fix that.
>> While there, rename BlockdevOptionsSheepdog member addr to server, for
>> consistency with BlockdevOptionsSsh, BlockdevOptionsGluster,
>> BlockdevOptionsNbd.
>> 
>> Commit 831acdc's example becomes
>> 
>>     --drive driver=sheepdog,server.type=inet,server.host=fido,server.port=7000,vdi=dolly
>> 
>> instead of
>> 
>>     --drive driver=sheepdog,host=fido,vdi=dolly
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/sheepdog.c     | 84 ++++++++++++++++++++++++++++++++++------------------
>>  qapi/block-core.json |  4 +--
>>  2 files changed, 58 insertions(+), 30 deletions(-)
>
> There's trailing whitespace somewhere in this patch (git-am complained).

Fixe in my tree.

>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index 89e98ed..c81013d 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -13,9 +13,11 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> +#include "qapi-visit.h"
>>  #include "qapi/error.h"
>>  #include "qapi/qmp/qdict.h"
>>  #include "qapi/qmp/qint.h"
>> +#include "qapi/qobject-input-visitor.h"
>>  #include "qemu/uri.h"
>>  #include "qemu/error-report.h"
>>  #include "qemu/sockets.h"
>> @@ -547,6 +549,47 @@ static SocketAddress *sd_socket_address(const char *path,
>>      return addr;
>>  }
>>  
>> +static SocketAddress *sd_server_config(QDict *options, Error **errp)
>> +{
>> +    QDict *server = NULL;
>> +    QObject *crumpled_server = NULL;
>> +    Visitor *iv = NULL;
>> +    SocketAddressFlat *saddr_flat = NULL;
>> +    SocketAddress *saddr = NULL;
>> +    Error *local_err = NULL;
>> +
>> +    qdict_extract_subqdict(options, &server, "server.");
>
> server may be empty:
>
> $ ./qemu-img info sheepdog:vdi
> qemu-img: Could not open 'sheepdog:vdi': Parameter 'type' is missing
>
> I'd propose creating an 'inet' SocketAddress object then for
> SD_DEFAULT_ADDR and SD_DEFAULT_PORT so that existing behavior won't change.

Makes sense.

In my working tree:

    $ upstream-qemu-img info sheepdog:vdi
    @@@ server.host=localhost
    @@@ server.port=7000
    @@@ tag=
    @@@ server.type=inet
    @@@ vdi=vdi
    ### vdi=vdi addr=localhost:7000 snap-id=(null) tag=
    upstream-qemu-img: Could not open 'sheepdog:vdi': Failed to connect socket: Connection refused

>> +
>> +    crumpled_server = qdict_crumple(server, errp);
>> +    if (!crumpled_server) {
>> +        goto done;
>> +    }
>> +
>> +    /*
>> +     * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive
>> +     * server.type=inet.  .to doesn't matter, it's ignored anyway.
>> +     * That's because when @options come from -blockdev or
>> +     * blockdev_add, members are typed according to the QAPI schema,
>> +     * but when they come from -drive, they're all QString.  The
>> +     * visitor expects the former.
>> +     */
>> +    iv = qobject_input_visitor_new(crumpled_server);
>> +    visit_type_SocketAddressFlat(iv, NULL, &saddr_flat, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        goto done;
>> +    }
>> +
>> +    saddr = socket_address_crumple(saddr_flat);
>> +
>> +done:
>> +    qapi_free_SocketAddressFlat(saddr_flat);
>> +    visit_free(iv);
>> +    qobject_decref(crumpled_server);
>> +    QDECREF(server);
>> +    return saddr;
>> +}
>> +
>>  /* Return -EIO in case of error, file descriptor on success */
>>  static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
>>  {
>> @@ -1175,15 +1218,17 @@ static void sd_parse_filename(const char *filename, QDict *options,
>>      }
>>  
>>      if (cfg.host) {
>> -        qdict_set_default_str(options, "host", cfg.host);
>> +        qdict_set_default_str(options, "server.host", cfg.host);
>> +        qdict_set_default_str(options, "server.type", "inet");
>>      }
>>      if (cfg.port) {
>>          snprintf(buf, sizeof(buf), "%d", cfg.port);
>> -        qdict_set_default_str(options, "port", buf);
>> +        qdict_set_default_str(options, "server.port", buf);
>
> .port is mandatory, so doing this optionally results in:
>
> $ ./qemu-img info sheepdog://localhost/foo
> qemu-img: Could not open 'sheepdog://localhost/foo': Parameter 'port' is
> missing

In my working tree:

    $ upstream-qemu-img info sheepdog://localhost6/foo
    @@@ server.host=localhost6
    @@@ server.port=7000
    @@@ tag=
    @@@ server.type=inet
    @@@ vdi=foo
    ### vdi=foo addr=localhost6:7000 snap-id=(null) tag=
    upstream-qemu-img: Could not open 'sheepdog://localhost6/foo': Failed to connect socket: Connection refused
    $ upstream-qemu-img info sheepdog://:123/foo
    @@@ server.host=localhost
    @@@ server.port=123
    @@@ tag=
    @@@ server.type=inet
    @@@ vdi=foo
    ### vdi=foo addr=localhost:123 snap-id=(null) tag=
    upstream-qemu-img: Could not open 'sheepdog://:123/foo': Failed to connect socket: Connection refused
    $ upstream-qemu-img info sheepdog:///foo
    @@@ server.host=localhost
    @@@ server.port=7000
    @@@ tag=
    @@@ server.type=inet
    @@@ vdi=foo
    ### vdi=foo addr=localhost:7000 snap-id=(null) tag=
    upstream-qemu-img: Could not open 'sheepdog:///foo': Failed to connect socket: Connection refused

>>      }
>>      if (cfg.path) {
>> -        qdict_set_default_str(options, "path", cfg.path);
>> -    }
>> +        qdict_set_default_str(options, "server.path", cfg.path); 
>> +        qdict_set_default_str(options, "server.type", "unix");
>> +   }
>
> Indentation is off.

Fixing...

> Max

Thanks!

[...]

Re: [Qemu-devel] [Qemu-block] [RFC v2 for-2.9 00/10] Fixes and cleanups around SocketAddress

[Kashyap Chamarthy]

On Thu, Mar 30, 2017 at 03:14:57PM +0200, Markus Armbruster wrote:
> What makes this 2.9 material is the crash bug fixed in PATCH 2 and the
> QAPI/QMP interface cleanups in PATCH 7+8.
> 
> This is RFC because:
> 
> 1. To give you one more chance to ask for undocumented -drive
>    driver=nbd usage compatibility [PATCH 08+09].
> 
> 2. Another round of sheepdog tests is still in progress (with
>    Kashyap's help).
> 
> Max, please have a close look at PATCH 11, I hope this is what you
> meant when you asked for "parsing @server".
> 
> v2:
> * PATCH 01+02+04+05 unchanged.
> * PATCH 03 error message improved, blank line [Max]
> * PATCH 06 commit message typo [Max]
> * PATCH 07 new, factored out of old PATCH 7
> * PATCH 08-10 updated, please re-review
> 
> Markus Armbruster (10):
>   nbd sockets vnc: Mark problematic address family tests TODO
>   char: Fix socket with "type": "vsock" address
>   io vnc sockets: Clean up SocketAddressKind switches
>   block: Document -drive problematic code and bugs
>   gluster: Prepare for SocketAddressFlat extension
>   qapi-schema: SocketAddressFlat variants 'vsock' and 'fd'
>   sockets: New helper socket_address_crumple()
>   nbd: Tidy up blockdev-add interface
>   squash! nbd: Tidy up blockdev-add interface
>   sheepdog: Fix blockdev-add

FWIW:

Tested-by: Kashyap Chamarthy <kchamart@redhat.com>

Details:

Env
~~~

I prepared a two-node Sheepdog cluster with Corosync.  Setup info:

	https://kashyapc.fedorapeople.org/virt/sheepdog-qemu-corosync.txt

And used Markus' branch 'blockdev-fixes-debug':

    http://repo.or.cz/qemu/armbru.git/tree/refs/heads/blockdev-fixes-dbg

    $ git describe
    pull-misc-2017-03-28-27-g885750d
    
    $ ./qemu-system-x86_64 -version
    QEMU emulator version 2.8.92 (v2.9.0-rc1-110-g885750d)
    Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers

Tests
~~~~~

There are four of them (thanks Markus):

[OK] Test-1: Syntax: sheepdog:hostname:port:Alice
[OK] Test-2: Syntax: sheepdog://hostname:port/Alice
[OK] Test-3: Syntax: -drive if=none,driver=sheepdog,server.type=inet,server.host=hostname,server.port=port,vdi=Alice
[OK] Test-4: Syntax: -blockdev node-name=nn,driver=sheepdog,server.type=inet,server.host=host,server.port=port,vdi=Alice

Complete test invocation 
~~~~~~~~~~~~~~~~~~~~~~~~

Test-1: Syntax: sheepdog:hostname:port:Alice
-----------------------------------------------------------------------
$ ./qemu-system-x86_64 -display none -nodefconfig -nodefaults sheepdog:192.168.122.84:7000:Alice
@@@ server.host=192.168.122.84
@@@ server.port=7000
@@@ tag=
@@@ server.type=inet
@@@ vdi=Alice
### vdi=Alice addr=192.168.122.84:7000 snap-id=(null) tag=
WARNING: Image format was not specified for 'json:{"server.host": "192.168.122.84", "server.port": "7000", "tag": "", "driver": "sheepdog", "server.type": "inet", "vdi": "Alice"}' and probing guessed raw.
         Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
         Specify the 'raw' format explicitly to remove the restrictions.
-----------------------------------------------------------------------

Test-2: Syntax: sheepdog://hostname:port/Alice
-----------------------------------------------------------------------
$ ./qemu-system-x86_64 -display none -nodefconfig -nodefaults sheepdog://192.168.122.84:7000/Alice
@@@ server.host=192.168.122.84
@@@ server.port=7000
@@@ tag=
@@@ server.type=inet
@@@ vdi=Alice
### vdi=Alice addr=192.168.122.84:7000 snap-id=(null) tag=
WARNING: Image format was not specified for 'json:{"server.host": "192.168.122.84", "server.port": "7000", "tag": "", "driver": "sheepdog", "server.type": "inet", "vdi": "Alice"}' and probing guessed raw.
         Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
         Specify the 'raw' format explicitly to remove the restrictions.
-----------------------------------------------------------------------

Test-3: Syntax: -drive \
if=none,driver=sheepdog,server.type=inet,server.host=hostname,server.port=port,vdi=Alice
-----------------------------------------------------------------------
$ ./qemu-system-x86_64 -display none -nodefconfig -nodefaults -drive if=none,driver=sheepdog,server.type=inet,server.host=192.168.122.84,server.port=7000,vdi=Alice
@@@ server.host=192.168.122.84
@@@ server.port=7000
@@@ server.type=inet
@@@ vdi=Alice
### vdi=Alice addr=192.168.122.84:7000 snap-id=(null) tag=
-----------------------------------------------------------------------

Test-4: Syntax: -blockdev \
node-name=nn,driver=sheepdog,server.type=inet,server.host=host,server.port=port,vdi=Alice
-----------------------------------------------------------------------
$ ./qemu-system-x86_64 -display none -nodefconfig -nodefaults -blockdev node-name=nn,driver=sheepdog,server.type=inet,server.host=192.168.122.84,server.port=7000,vdi=Alice
@@@ server.port=7000
@@@ server.host=192.168.122.84
@@@ server.type=inet
@@@ vdi=Alice
### vdi=Alice addr=192.168.122.84:7000 snap-id=(null) tag=
-----------------------------------------------------------------------


-- 
/kashyap

Re: [Qemu-devel] [Qemu-block] [RFC v2 for-2.9 00/10] Fixes and cleanups around SocketAddress

[Markus Armbruster]

Kashyap Chamarthy <kchamart@redhat.com> writes:

> On Thu, Mar 30, 2017 at 03:14:57PM +0200, Markus Armbruster wrote:
>> What makes this 2.9 material is the crash bug fixed in PATCH 2 and the
>> QAPI/QMP interface cleanups in PATCH 7+8.
>> 
>> This is RFC because:
>> 
>> 1. To give you one more chance to ask for undocumented -drive
>>    driver=nbd usage compatibility [PATCH 08+09].
>> 
>> 2. Another round of sheepdog tests is still in progress (with
>>    Kashyap's help).
>> 
>> Max, please have a close look at PATCH 11, I hope this is what you
>> meant when you asked for "parsing @server".
>> 
>> v2:
>> * PATCH 01+02+04+05 unchanged.
>> * PATCH 03 error message improved, blank line [Max]
>> * PATCH 06 commit message typo [Max]
>> * PATCH 07 new, factored out of old PATCH 7
>> * PATCH 08-10 updated, please re-review
>> 
>> Markus Armbruster (10):
>>   nbd sockets vnc: Mark problematic address family tests TODO
>>   char: Fix socket with "type": "vsock" address
>>   io vnc sockets: Clean up SocketAddressKind switches
>>   block: Document -drive problematic code and bugs
>>   gluster: Prepare for SocketAddressFlat extension
>>   qapi-schema: SocketAddressFlat variants 'vsock' and 'fd'
>>   sockets: New helper socket_address_crumple()
>>   nbd: Tidy up blockdev-add interface
>>   squash! nbd: Tidy up blockdev-add interface
>>   sheepdog: Fix blockdev-add
>
> FWIW:
>
> Tested-by: Kashyap Chamarthy <kchamart@redhat.com>
>
> Details:
>
> Env
> ~~~
>
> I prepared a two-node Sheepdog cluster with Corosync.  Setup info:
>
> 	https://kashyapc.fedorapeople.org/virt/sheepdog-qemu-corosync.txt
>
> And used Markus' branch 'blockdev-fixes-debug':

blockdev-fixes-dbg, actually.  Its this series plus a few debug prints,
diffs appended.

>
>     http://repo.or.cz/qemu/armbru.git/tree/refs/heads/blockdev-fixes-dbg
>
>     $ git describe
>     pull-misc-2017-03-28-27-g885750d
>     
>     $ ./qemu-system-x86_64 -version
>     QEMU emulator version 2.8.92 (v2.9.0-rc1-110-g885750d)
>     Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers
>
> Tests
> ~~~~~
>
> There are four of them (thanks Markus):
>
> [OK] Test-1: Syntax: sheepdog:hostname:port:Alice

The old -drive pseudo-filename syntax.

> [OK] Test-2: Syntax: sheepdog://hostname:port/Alice

The new -drive pseudo-filename syntax.

> [OK] Test-3: Syntax: -drive if=none,driver=sheepdog,server.type=inet,server.host=hostname,server.port=port,vdi=Alice

Unsugared -drive syntax.

> [OK] Test-4: Syntax: -blockdev node-name=nn,driver=sheepdog,server.type=inet,server.host=host,server.port=port,vdi=Alice

-blockdev, obviously.

> Complete test invocation 
> ~~~~~~~~~~~~~~~~~~~~~~~~
>
> Test-1: Syntax: sheepdog:hostname:port:Alice
> -----------------------------------------------------------------------
> $ ./qemu-system-x86_64 -display none -nodefconfig -nodefaults sheepdog:192.168.122.84:7000:Alice
> @@@ server.host=192.168.122.84
> @@@ server.port=7000
> @@@ tag=
> @@@ server.type=inet
> @@@ vdi=Alice
> ### vdi=Alice addr=192.168.122.84:7000 snap-id=(null) tag=
> WARNING: Image format was not specified for 'json:{"server.host": "192.168.122.84", "server.port": "7000", "tag": "", "driver": "sheepdog", "server.type": "inet", "vdi": "Alice"}' and probing guessed raw.
>          Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
>          Specify the 'raw' format explicitly to remove the restrictions.
> -----------------------------------------------------------------------
>
> Test-2: Syntax: sheepdog://hostname:port/Alice
> -----------------------------------------------------------------------
> $ ./qemu-system-x86_64 -display none -nodefconfig -nodefaults sheepdog://192.168.122.84:7000/Alice
> @@@ server.host=192.168.122.84
> @@@ server.port=7000
> @@@ tag=
> @@@ server.type=inet
> @@@ vdi=Alice
> ### vdi=Alice addr=192.168.122.84:7000 snap-id=(null) tag=
> WARNING: Image format was not specified for 'json:{"server.host": "192.168.122.84", "server.port": "7000", "tag": "", "driver": "sheepdog", "server.type": "inet", "vdi": "Alice"}' and probing guessed raw.
>          Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
>          Specify the 'raw' format explicitly to remove the restrictions.
> -----------------------------------------------------------------------
>
> Test-3: Syntax: -drive \
> if=none,driver=sheepdog,server.type=inet,server.host=hostname,server.port=port,vdi=Alice
> -----------------------------------------------------------------------
> $ ./qemu-system-x86_64 -display none -nodefconfig -nodefaults -drive if=none,driver=sheepdog,server.type=inet,server.host=192.168.122.84,server.port=7000,vdi=Alice
> @@@ server.host=192.168.122.84
> @@@ server.port=7000
> @@@ server.type=inet
> @@@ vdi=Alice
> ### vdi=Alice addr=192.168.122.84:7000 snap-id=(null) tag=
> -----------------------------------------------------------------------
>
> Test-4: Syntax: -blockdev \
> node-name=nn,driver=sheepdog,server.type=inet,server.host=host,server.port=port,vdi=Alice
> -----------------------------------------------------------------------
> $ ./qemu-system-x86_64 -display none -nodefconfig -nodefaults -blockdev node-name=nn,driver=sheepdog,server.type=inet,server.host=192.168.122.84,server.port=7000,vdi=Alice
> @@@ server.port=7000
> @@@ server.host=192.168.122.84
> @@@ server.type=inet
> @@@ vdi=Alice
> ### vdi=Alice addr=192.168.122.84:7000 snap-id=(null) tag=
> -----------------------------------------------------------------------

Thanks a lot for your help with testing!


diff --git a/block/nbd.c b/block/nbd.c
index 8bb29a9..9b3e361 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -41,6 +41,44 @@
 #include "qapi/qmp/qstring.h"
 #include "qemu/cutils.h"
 
+#include "qapi/qmp/qbool.h"
+#include "qapi/qmp/qfloat.h"
+
+static void qdict_print(QDict *qdict)
+{
+    const QDictEntry *entry;
+    for (entry = qdict_first(qdict); entry; entry = qdict_next(qdict, entry)) {
+        printf("@@@ %s=", entry->key);
+        switch (qobject_type(entry->value)) {
+        default:
+            printf("crap\n");
+            break;
+        case QTYPE_QNULL:
+            printf("null\n");
+            break;
+        case QTYPE_QINT:
+            printf("%" PRId64 "\n", qint_get_int(qobject_to_qint(entry->value)));
+            break;
+        case QTYPE_QSTRING:
+            printf("%s\n", qstring_get_str(qobject_to_qstring(entry->value)));
+            break;
+        case QTYPE_QDICT:
+            printf("dict\n");
+            break;
+        case QTYPE_QLIST:
+            printf("list\n");
+            break;
+        case QTYPE_QFLOAT:
+            printf("%g\n", qfloat_get_double(qobject_to_qfloat(entry->value)));
+            break;
+        case QTYPE_QBOOL:
+            printf("%s\n", qbool_get_bool(qobject_to_qbool(entry->value)) ? "true" : "false");
+            break;
+        }
+    }
+
+}
+
 #define EN_OPTSTR ":exportname="
 
 typedef struct BDRVNBDState {
@@ -405,6 +443,8 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     const char *hostname = NULL;
     int ret = -EINVAL;
 
+    qdict_print(options);
+
     opts = qemu_opts_create(&nbd_runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (local_err) {
@@ -449,6 +489,23 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto error;
     }
 
+    printf("### type=%u\n", s->saddr->type);
+    switch (s->saddr->type) {
+    case SOCKET_ADDRESS_KIND_INET:
+        printf("    host=%s\n    port=%s\n",
+               s->saddr->u.inet.host,
+               s->saddr->u.inet.port);
+        break;
+    case SOCKET_ADDRESS_KIND_UNIX:
+        printf("    path=%s\n",
+               s->saddr->u.q_unix.path);
+        break;
+    default:
+        ;
+    }
+    printf("    export=%s\n    tls-creds=%s\n",
+           s->export, s->tlscredsid);
+
     /* NBD handshake */
     ret = nbd_client_init(bs, sioc, s->export,
                           tlscreds, hostname, errp);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index c81013d..a9ed709 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -26,6 +26,47 @@
 #include "qemu/bitops.h"
 #include "qemu/cutils.h"
 
+#include "qapi/qmp/qbool.h"
+#include "qapi/qmp/qint.h"
+#include "qapi/qmp/qfloat.h"
+#include "qapi/qmp/qstring.h"
+
+static void qdict_print(QDict *qdict)
+{
+    const QDictEntry *entry;
+    for (entry = qdict_first(qdict); entry; entry = qdict_next(qdict, entry)) {
+        printf("@@@ %s=", entry->key);
+        switch (qobject_type(entry->value)) {
+        default:
+            printf("crap\n");
+            break;
+        case QTYPE_QNULL:
+            printf("null\n");
+            break;
+        case QTYPE_QINT:
+            printf("%" PRId64 "\n", qint_get_int(qobject_to_qint(entry->value)));
+            break;
+        case QTYPE_QSTRING:
+            printf("%s\n", qstring_get_str(qobject_to_qstring(entry->value)));
+            break;
+        case QTYPE_QDICT:
+            printf("dict\n");
+            break;
+        case QTYPE_QLIST:
+            printf("list\n");
+            break;
+        case QTYPE_QFLOAT:
+            printf("%g\n", qfloat_get_double(qobject_to_qfloat(entry->value)));
+            break;
+        case QTYPE_QBOOL:
+            printf("%s\n", qbool_get_bool(qobject_to_qbool(entry->value)) ? "true" : "false");
+            break;
+        }
+    }
+
+}
+
+
 #define SD_PROTO_VER 0x01
 
 #define SD_DEFAULT_ADDR "localhost"
@@ -1582,6 +1623,8 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     QemuOpts *opts;
     Error *local_err = NULL;
 
+    qdict_print(options);
+
     s->bs = bs;
     s->aio_context = bdrv_get_aio_context(bs);
 
@@ -1634,6 +1677,13 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
         goto err_no_fd;
     }
 
+    {
+        char *addr = socket_address_to_string(s->addr, &error_abort);
+        printf("### vdi=%s addr=%s snap-id=%s tag=%s\n",
+           vdi, addr, snap_id_str, tag);
+        g_free(addr);
+    }
+
     QLIST_INIT(&s->inflight_aio_head);
     QLIST_INIT(&s->failed_aio_head);
     QLIST_INIT(&s->inflight_aiocb_head);