[Qemu-devel] [for-2.9 0/8] Fixes and cleanups around SocketAddress

[Qemu-devel] [for-2.9 0/8] 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.

Sheepdog tests still in progress (with Kashyap's help), but I'm
sending this out now in the hope of getting a head start on review.

Markus Armbruster (8):
  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'
  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           | 140 ++++++++++++++++++++++++++++++++++++++++----------
 block/nfs.c           |   7 +++
 block/rbd.c           |   6 +++
 block/sheepdog.c      |  18 +++----
 block/ssh.c           |   8 +++
 blockdev-nbd.c        |   1 +
 chardev/char-socket.c |   9 ++--
 io/dns-resolver.c     |   6 ++-
 qapi-schema.json      |  19 +++----
 qapi/block-core.json  |   6 +--
 ui/vnc.c              |  11 +++-
 util/qemu-sockets.c   |   8 +--
 15 files changed, 241 insertions(+), 73 deletions(-)

-- 
2.7.4

[Qemu-devel] [for-2.9 1/8] 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>
---
 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

Re: [Qemu-devel] [for-2.9 1/8] nbd sockets vnc: Mark problematic address family tests TODO

[Max Reitz]

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

On 29.03.2017 18:45, Markus Armbruster wrote:
> 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>
> ---
>  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(-)

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


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

Re: [Qemu-devel] [for-2.9 1/8] nbd sockets vnc: Mark problematic address family tests TODO

[Eric Blake]

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

On 03/29/2017 11:45 AM, Markus Armbruster wrote:
> 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: 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 --]

[Qemu-devel] [for-2.9 2/8] 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>
---
 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

Re: [Qemu-devel] [for-2.9 2/8] char: Fix socket with "type": "vsock" address

[Marc-André Lureau]

Hi

----- Original Message -----
> 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>

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

ooch.. may I suggest we don't abort() here? g_return_val_if_fail() perhaps a more judicious choice?

Re: [Qemu-devel] [for-2.9 2/8] char: Fix socket with "type": "vsock" address

[Marc-André Lureau]

----- Original Message -----
> 
> Hi
> 
> ----- Original Message -----
> > 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>
> 
> > ---
> >  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();
> 
> ooch.. may I suggest we don't abort() here? g_return_val_if_fail() perhaps a
> more judicious choice?

-> g_return_if_reached()

> 

Re: [Qemu-devel] [for-2.9 2/8] char: Fix socket with "type": "vsock" address

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> ----- Original Message -----
>> 
>> Hi
>> 
>> ----- Original Message -----
>> > 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>

Thanks!

>> > ---
>> >  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();
>> 
>> ooch.. may I suggest we don't abort() here? g_return_val_if_fail() perhaps a
>> more judicious choice?
>
> -> g_return_if_reached()

If we reach the default label, program state is messed up, and
continuing is unsafe.

In case we decide we want to push our luck and continue anyway: as far
as I can tell, we don't use g_log() anywhere.  We can discuss whether we
have uses for it, but hard freeze is a bad time to actually introduce
it.

Re: [Qemu-devel] [for-2.9 2/8] char: Fix socket with "type": "vsock" address

[Stefan Hajnoczi]

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

On Wed, Mar 29, 2017 at 06:45:14PM +0200, Markus Armbruster wrote:
> 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>
> ---
>  chardev/char-socket.c | 4 ++++
>  1 file changed, 4 insertions(+)

Thanks for the fix!

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [for-2.9 2/8] char: Fix socket with "type": "vsock" address

[Max Reitz]

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

On 29.03.2017 18:45, Markus Armbruster wrote:
> 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>
> ---
>  chardev/char-socket.c | 4 ++++
>  1 file changed, 4 insertions(+)

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


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

[Qemu-devel] [for-2.9 3/8] 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.

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

diff --git a/io/dns-resolver.c b/io/dns-resolver.c
index 0ac6b23..00fb575 100644
--- a/io/dns-resolver.c
+++ b/io/dns-resolver.c
@@ -164,9 +164,11 @@ 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..b6b58c4 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:
+    case SOCKET_ADDRESS_KIND_VSOCK:
+    case SOCKET_ADDRESS_KIND_FD:
         error_setg(errp, "Unsupported socket kind %d",
                    addr->type);
         break;
+    default:
+        abort();
     }
 
     return;
@@ -411,10 +414,13 @@ VncInfo *qmp_query_vnc(Error **errp)
             info->family = NETWORK_ADDRESS_FAMILY_UNIX;
             break;
 
-        default:
+        case SOCKET_ADDRESS_KIND_VSOCK:
+        case SOCKET_ADDRESS_KIND_FD:
             error_setg(errp, "Unsupported socket kind %d",
                        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] [for-2.9 3/8] io vnc sockets: Clean up SocketAddressKind switches

[Max Reitz]

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

On 29.03.2017 18:45, 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.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  io/dns-resolver.c   |  6 ++++--
>  ui/vnc.c            | 10 ++++++++--
>  util/qemu-sockets.c |  4 +---
>  3 files changed, 13 insertions(+), 7 deletions(-)

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

Nit-picks below, as can be expected of me.

> diff --git a/io/dns-resolver.c b/io/dns-resolver.c
> index 0ac6b23..00fb575 100644
> --- a/io/dns-resolver.c
> +++ b/io/dns-resolver.c
> @@ -164,9 +164,11 @@ 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;

Could do with an empty line here like the other cases have.

>      default:
> -        error_setg(errp, "Unknown socket address kind");
> -        return -1;
> +        abort();
>      }
>  }
>  
> diff --git a/ui/vnc.c b/ui/vnc.c
> index fe0a46a..b6b58c4 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:
> +    case SOCKET_ADDRESS_KIND_VSOCK:
> +    case SOCKET_ADDRESS_KIND_FD:
>          error_setg(errp, "Unsupported socket kind %d",
>                     addr->type);

Pre-existing, but: %s and SocketAddressKind_lookup[addr->type] would be
nicer.

>          break;
> +    default:
> +        abort();
>      }
>  
>      return;
> @@ -411,10 +414,13 @@ VncInfo *qmp_query_vnc(Error **errp)
>              info->family = NETWORK_ADDRESS_FAMILY_UNIX;
>              break;
>  
> -        default:
> +        case SOCKET_ADDRESS_KIND_VSOCK:
> +        case SOCKET_ADDRESS_KIND_FD:
>              error_setg(errp, "Unsupported socket kind %d",
>                         addr->type);

Same here.

Max

>              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;
>  }
> 



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

Re: [Qemu-devel] [for-2.9 3/8] io vnc sockets: Clean up SocketAddressKind switches

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

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

Thanks!

> Nit-picks below, as can be expected of me.

Bring 'em on!

>> diff --git a/io/dns-resolver.c b/io/dns-resolver.c
>> index 0ac6b23..00fb575 100644
>> --- a/io/dns-resolver.c
>> +++ b/io/dns-resolver.c
>> @@ -164,9 +164,11 @@ 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;
>
> Could do with an empty line here like the other cases have.

Indeed.

>>      default:
>> -        error_setg(errp, "Unknown socket address kind");
>> -        return -1;
>> +        abort();
>>      }
>>  }
>>  
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index fe0a46a..b6b58c4 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:
>> +    case SOCKET_ADDRESS_KIND_VSOCK:
>> +    case SOCKET_ADDRESS_KIND_FD:
>>          error_setg(errp, "Unsupported socket kind %d",
>>                     addr->type);
>
> Pre-existing, but: %s and SocketAddressKind_lookup[addr->type] would be
> nicer.

Can do.

>>          break;
>> +    default:
>> +        abort();
>>      }
>>  
>>      return;
>> @@ -411,10 +414,13 @@ VncInfo *qmp_query_vnc(Error **errp)
>>              info->family = NETWORK_ADDRESS_FAMILY_UNIX;
>>              break;
>>  
>> -        default:
>> +        case SOCKET_ADDRESS_KIND_VSOCK:
>> +        case SOCKET_ADDRESS_KIND_FD:
>>              error_setg(errp, "Unsupported socket kind %d",
>>                         addr->type);
>
> Same here.

Yup.

> Max
>
>>              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;
>>  }
>> 

Re: [Qemu-devel] [for-2.9 3/8] io vnc sockets: Clean up SocketAddressKind switches

[Daniel P. Berrange]

On Wed, Mar 29, 2017 at 06:45:15PM +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.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  io/dns-resolver.c   |  6 ++++--
>  ui/vnc.c            | 10 ++++++++--
>  util/qemu-sockets.c |  4 +---
>  3 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/io/dns-resolver.c b/io/dns-resolver.c
> index 0ac6b23..00fb575 100644
> --- a/io/dns-resolver.c
> +++ b/io/dns-resolver.c
> @@ -164,9 +164,11 @@ 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();
>      }
>  }

KIND_FD needs to be handled the same way as KIND_UNIX/VSOCK - ie a no-op,
not 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/ :|

[Qemu-devel] [for-2.9 4/8] 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] [for-2.9 4/8] block: Document -drive problematic code and bugs

[Max Reitz]

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

On 29.03.2017 18:45, Markus Armbruster wrote:
> -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(-)

I have to say I don't like how the comments in block.c are completely
generic.

> 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");

For instance this one: Well, yes, for -drive, this will always be a
QString. Which is OK, because that's what we're trying to get.

The comment makes this confusing, IMO. If you really want a comment here
it should at least contain a mention that it's totally fine in practice
here. Calling the code "problematic" sounds like this could blow up when
it reality it can't; and I would think it actually is the most sane
solution given the current state of the whole infrastructure (i.e. how
-drive and -blockdev work).

Same for all the other plain qdict_get_try_str() calls (in block.c, at
least).

>      }
>  
> @@ -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.
> +     */

Now this one should mention that this is the very reason why we are
attempting parsing the QDict string before getting a boolean value directly.

>      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");

This comment I'm very much OK with, though, because we should not
retrieve runtime options directly from the QDict.

(Same for the comments in the other block drivers.)

Max

>      char bsd_path[MAXPATHLEN] = "";
>      bool error_occurred = false;



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

Re: [Qemu-devel] [for-2.9 4/8] block: Document -drive problematic code and bugs

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 29.03.2017 18:45, Markus Armbruster wrote:
>> -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(-)
>
> I have to say I don't like how the comments in block.c are completely
> generic.

I'm afraid they reflect my rather "generic" understanding of block.c.

>> 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");
>
> For instance this one: Well, yes, for -drive, this will always be a
> QString. Which is OK, because that's what we're trying to get.
>
> The comment makes this confusing, IMO. If you really want a comment here
> it should at least contain a mention that it's totally fine in practice
> here. Calling the code "problematic" sounds like this could blow up when
> it reality it can't; and I would think it actually is the most sane
> solution given the current state of the whole infrastructure (i.e. how
> -drive and -blockdev work).

Well, if it could blow up, I'd call it wrong, and start the comment with
FIXME :)

Even though qdict_get_try_str() is indeed fine, I propose to have a
comment, because someone with less detailed understanding of how the
configuration machine works (me, until yesterday, and probably again
after next month) could conclude that qdict_get_try_bool() would be just
as fine.

What about:

   /*
    * Caution: while qdict_get_try_str() is fine, getting non-string
    * types would require more care.  When @options come from -blockdev
    * or blockdev_add, its members are typed according to the QAPI
    * schema, but when they come from -drive, they're all QString.
    */

If you don't like this comment either, care to suggest one you do?

> Same for all the other plain qdict_get_try_str() calls (in block.c, at
> least).
>
>>      }
>>  
>> @@ -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.
>> +     */
>
> Now this one should mention that this is the very reason why we are
> attempting parsing the QDict string before getting a boolean value directly.

What about:

        /*
         * Set the BDRV_O_RDWR and BDRV_O_ALLOW_RDWR flags.
         * Caution: getting a boolean member of @options requires care.
         * 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.
         */

>>      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");
>
> This comment I'm very much OK with, though, because we should not
> retrieve runtime options directly from the QDict.
>
> (Same for the comments in the other block drivers.)
>
> Max
>
>>      char bsd_path[MAXPATHLEN] = "";
>>      bool error_occurred = false;

Re: [Qemu-devel] [for-2.9 4/8] block: Document -drive problematic code and bugs

[Eric Blake]

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

On 03/30/2017 01:52 AM, Markus Armbruster wrote:

>>> +++ 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");
>>
>> For instance this one: Well, yes, for -drive, this will always be a
>> QString. Which is OK, because that's what we're trying to get.
>>
>> The comment makes this confusing, IMO. If you really want a comment here
>> it should at least contain a mention that it's totally fine in practice
>> here. Calling the code "problematic" sounds like this could blow up when
>> it reality it can't; and I would think it actually is the most sane
>> solution given the current state of the whole infrastructure (i.e. how
>> -drive and -blockdev work).
> 
> Well, if it could blow up, I'd call it wrong, and start the comment with
> FIXME :)
> 
> Even though qdict_get_try_str() is indeed fine, I propose to have a
> comment, because someone with less detailed understanding of how the
> configuration machine works (me, until yesterday, and probably again
> after next month) could conclude that qdict_get_try_bool() would be just
> as fine.
> 
> What about:
> 
>    /*
>     * Caution: while qdict_get_try_str() is fine, getting non-string
>     * types would require more care.  When @options come from -blockdev
>     * or blockdev_add, its members are typed according to the QAPI
>     * schema, but when they come from -drive, they're all QString.
>     */

Yes, that's better - it makes it obvious that our current usage works,
but that the code must not be carelessly edited if we add another field
in the future.

-- 
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] [for-2.9 4/8] block: Document -drive problematic code and bugs

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/30/2017 01:52 AM, Markus Armbruster wrote:
>
>>>> +++ 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");
>>>
>>> For instance this one: Well, yes, for -drive, this will always be a
>>> QString. Which is OK, because that's what we're trying to get.
>>>
>>> The comment makes this confusing, IMO. If you really want a comment here
>>> it should at least contain a mention that it's totally fine in practice
>>> here. Calling the code "problematic" sounds like this could blow up when
>>> it reality it can't; and I would think it actually is the most sane
>>> solution given the current state of the whole infrastructure (i.e. how
>>> -drive and -blockdev work).
>> 
>> Well, if it could blow up, I'd call it wrong, and start the comment with
>> FIXME :)
>> 
>> Even though qdict_get_try_str() is indeed fine, I propose to have a
>> comment, because someone with less detailed understanding of how the
>> configuration machine works (me, until yesterday, and probably again
>> after next month) could conclude that qdict_get_try_bool() would be just
>> as fine.
>> 
>> What about:
>> 
>>    /*
>>     * Caution: while qdict_get_try_str() is fine, getting non-string
>>     * types would require more care.  When @options come from -blockdev
>>     * or blockdev_add, its members are typed according to the QAPI
>>     * schema, but when they come from -drive, they're all QString.
>>     */
>
> Yes, that's better - it makes it obvious that our current usage works,
> but that the code must not be carelessly edited if we add another field
> in the future.

If Max is also happy with it, I'll put it in v3.

Re: [Qemu-devel] [for-2.9 4/8] block: Document -drive problematic code and bugs

[Max Reitz]

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

On 30.03.2017 16:42, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 03/30/2017 01:52 AM, Markus Armbruster wrote:
>>
>>>>> +++ 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");
>>>>
>>>> For instance this one: Well, yes, for -drive, this will always be a
>>>> QString. Which is OK, because that's what we're trying to get.
>>>>
>>>> The comment makes this confusing, IMO. If you really want a comment here
>>>> it should at least contain a mention that it's totally fine in practice
>>>> here. Calling the code "problematic" sounds like this could blow up when
>>>> it reality it can't; and I would think it actually is the most sane
>>>> solution given the current state of the whole infrastructure (i.e. how
>>>> -drive and -blockdev work).
>>>
>>> Well, if it could blow up, I'd call it wrong, and start the comment with
>>> FIXME :)
>>>
>>> Even though qdict_get_try_str() is indeed fine, I propose to have a
>>> comment, because someone with less detailed understanding of how the
>>> configuration machine works (me, until yesterday, and probably again
>>> after next month) could conclude that qdict_get_try_bool() would be just
>>> as fine.
>>>
>>> What about:
>>>
>>>    /*
>>>     * Caution: while qdict_get_try_str() is fine, getting non-string
>>>     * types would require more care.  When @options come from -blockdev
>>>     * or blockdev_add, its members are typed according to the QAPI
>>>     * schema, but when they come from -drive, they're all QString.
>>>     */
>>
>> Yes, that's better - it makes it obvious that our current usage works,
>> but that the code must not be carelessly edited if we add another field
>> in the future.
> 
> If Max is also happy with it, I'll put it in v3.

Yes, more than happy, thanks! (Same for the other comment.)

Max


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

[Qemu-devel] [for-2.9 5/8] 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>
---
 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

Re: [Qemu-devel] [for-2.9 5/8] gluster: Prepare for SocketAddressFlat extension

[Max Reitz]

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

On 29.03.2017 18:45, Markus Armbruster wrote:
> 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>
> ---
>  block/gluster.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)

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


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

Re: [Qemu-devel] [for-2.9 5/8] gluster: Prepare for SocketAddressFlat extension

[Jeff Cody]

On Wed, Mar 29, 2017 at 06:45:17PM +0200, Markus Armbruster wrote:
> 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>
> ---
>  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 type is not either _UNIX or _INET, something is broken, make sense.

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

Reviewed-by: Jeff Cody <jcody@redhat.com>

[Qemu-devel] [for-2.9 6/8] 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
'tcp' or 'unix'.

Signed-off-by: Markus Armbruster <armbru@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] [for-2.9 6/8] qapi-schema: SocketAddressFlat variants 'vsock' and 'fd'

[Max Reitz]

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

On 29.03.2017 18:45, 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
> 'tcp' or 'unix'.

s/tcp/inet/

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

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


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

Re: [Qemu-devel] [for-2.9 6/8] qapi-schema: SocketAddressFlat variants 'vsock' and 'fd'

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 29.03.2017 18:45, 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
>> 'tcp' or 'unix'.
>
> s/tcp/inet/

Fixing...

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

Thanks!

Re: [Qemu-devel] [for-2.9 6/8] qapi-schema: SocketAddressFlat variants 'vsock' and 'fd'

[Eric Blake]

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

On 03/29/2017 11:45 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
> 'tcp' or 'unix'.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/gluster.c  |  2 ++
>  qapi-schema.json | 19 ++++++++-----------
>  2 files changed, 10 insertions(+), 11 deletions(-)

> +# 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' } }

Can we make 'fd':'str'?  That would be even less pointless nesting on
the wire.

-- 
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] [for-2.9 6/8] qapi-schema: SocketAddressFlat variants 'vsock' and 'fd'

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/29/2017 11:45 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
>> 'tcp' or 'unix'.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/gluster.c  |  2 ++
>>  qapi-schema.json | 19 ++++++++-----------
>>  2 files changed, 10 insertions(+), 11 deletions(-)
>
>> +# 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' } }
>
> Can we make 'fd':'str'?  That would be even less pointless nesting on
> the wire.

I guess it's wrapped in an object here to keep the door open for
future extensions.  Perhaps also for symmetry.

Re: [Qemu-devel] [for-2.9 6/8] qapi-schema: SocketAddressFlat variants 'vsock' and 'fd'

[Eric Blake]

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

On 03/30/2017 01:55 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 03/29/2017 11:45 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
>>> 'tcp' or 'unix'.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  block/gluster.c  |  2 ++
>>>  qapi-schema.json | 19 ++++++++-----------
>>>  2 files changed, 10 insertions(+), 11 deletions(-)
>>
>>> +# 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' } }
>>
>> Can we make 'fd':'str'?  That would be even less pointless nesting on
>> the wire.
> 
> I guess it's wrapped in an object here to keep the door open for
> future extensions.  Perhaps also for symmetry.

And I just reminded myself of the restrictions on flat unions:

"A flat union definition avoids nesting on the wire, and specifies a
set of common members that occur in all variants of the union.  The
'base' key must specify either a type name (the type must be a
struct, not a union), or a dictionary representing an anonymous type.
All branches of the union must be complex types, "

So the end result is that as written, you pass "addr":{"type":"fd",
"str":"name_of_fd"}; and we can't use 'fd':'str' in the schema after all.

With the fix that Max pointed out,
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 --]

[Qemu-devel] [for-2.9 7/8] 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, but
it's new in 2.9.  Replace it by SocketAddressFlat while we can.  This
simplifies

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

to

    { "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          | 131 ++++++++++++++++++++++++++++++++++++++++-----------
 qapi/block-core.json |   2 +-
 2 files changed, 104 insertions(+), 29 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 11e3ba7..199cb06 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -33,6 +33,7 @@
 #include "block/block_int.h"
 #include "qemu/module.h"
 #include "qapi-visit.h"
+#include "qapi/clone-visitor.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qobject-output-visitor.h"
 #include "qapi/qmp/qdict.h"
@@ -47,7 +48,7 @@ typedef struct BDRVNBDState {
     NBDClientSession client;
 
     /* For nbd_refresh_filename() */
-    SocketAddress *saddr;
+    SocketAddressFlat *saddr;
     char *export, *tlscredsid;
 } BDRVNBDState;
 
@@ -95,7 +96,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 +117,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 +198,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 +208,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,17 +224,45 @@ 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 = qemu_opt_get(legacy_opts, "server.data.path");
+    const char *sd_host = qemu_opt_get(legacy_opts, "server.data.host");
+    const char *sd_port = qemu_opt_get(legacy_opts, "server.data.port");
+    bool no_server = path || host || port;
+    bool server_data = sd_path || sd_host || sd_port;
     const QDictEntry *e;
 
-    if (!path && !host && !port) {
+    if (!no_server && !server_data) {
+        return true;
+    }
+
+    if (no_server && server_data) {
+        error_setg(errp, "Cannot use %s and %s at the same time",
+                   "path/host/port", "server.data.*");
+        return false;
+    }
+
+    if (server_data) {
+        if (sd_host) {
+            qdict_put(output_options, "server.host",
+                      qstring_from_str(sd_host));
+        }
+        if (sd_port) {
+            qdict_put(output_options, "server.port",
+                      qstring_from_str(sd_port));
+        }
+        if (sd_path) {
+            qdict_put(output_options, "server.path",
+                      qstring_from_str(sd_path));
+        }
         return true;
     }
 
     for (e = qdict_first(output_options); e; e = qdict_next(output_options, e))
     {
         if (strstart(e->key, "server.", NULL)) {
-            error_setg(errp, "Cannot use 'server' and path/host/port at the "
-                       "same time");
+            error_setg(errp, "Cannot use %s and %s at the same time",
+                       e->key,
+                       no_server ? "path/host/port" : "server.data.*");
             return false;
         }
     }
@@ -248,20 +277,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 +317,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 +336,38 @@ NBDClientSession *nbd_get_client_session(BlockDriverState *bs)
     return &s->client;
 }
 
-static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
+static SocketAddress *socket_address_crumple(SocketAddressFlat *saddr_flat)
+{
+    SocketAddress *saddr = g_new(SocketAddress, 1);
+
+    saddr->type = saddr_flat->type;
+    switch (saddr->type) {
+    case SOCKET_ADDRESS_FLAT_TYPE_INET:
+        saddr->u.inet.data = QAPI_CLONE(InetSocketAddress,
+                                        &saddr_flat->u.inet);
+        break;
+    case SOCKET_ADDRESS_FLAT_TYPE_UNIX:
+        saddr->u.q_unix.data = QAPI_CLONE(UnixSocketAddress, &
+                                          saddr_flat->u.q_unix);
+        break;
+    case SOCKET_ADDRESS_FLAT_TYPE_VSOCK:
+        saddr->u.vsock.data = QAPI_CLONE(VsockSocketAddress,
+                                         &saddr_flat->u.vsock);
+        break;
+    case SOCKET_ADDRESS_FLAT_TYPE_FD:
+        saddr->u.fd.data = QAPI_CLONE(String, &saddr_flat->u.fd);
+        break;
+    default:
+        abort();
+    }
+
+    return 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 +377,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;
@@ -379,6 +439,21 @@ static QemuOptsList nbd_runtime_opts = {
             .help = "Unix socket path to connect to",
         },
         {
+            .name = "server.data.host",
+            .type = QEMU_OPT_STRING,
+            .help = "TCP host to connect to",
+        },
+        {
+            .name = "server.data.port",
+            .type = QEMU_OPT_STRING,
+            .help = "TCP port to connect to",
+        },
+        {
+            .name = "server.data.path",
+            .type = QEMU_OPT_STRING,
+            .help = "Unix socket path to connect to",
+        },
+        {
             .name = "export",
             .type = QEMU_OPT_STRING,
             .help = "Name of the NBD export to open",
@@ -409,7 +484,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 +505,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 +532,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 +558,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 +589,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 +616,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] [for-2.9 7/8] nbd: Tidy up blockdev-add interface

[Paolo Bonzini]

On 29/03/2017 18:45, Markus Armbruster wrote:
> 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

I think it's reasonable to assume nobody has used this and break it.

Paolo

Re: [Qemu-devel] [for-2.9 7/8] nbd: Tidy up blockdev-add interface

[Max Reitz]

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

On 29.03.2017 18:45, 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, but
> it's new in 2.9.  Replace it by SocketAddressFlat while we can.  This
> simplifies
> 
>     { "execute": "blockdev-add",
>       "arguments": { "node-name": "foo", "driver": "nbd",
>                      "server": { "type": "inet",
> 		                 "data": { "host": "localhost",
> 				           "port": "12345" } } } }
> 
> to
> 
>     { "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          | 131 ++++++++++++++++++++++++++++++++++++++++-----------
>  qapi/block-core.json |   2 +-
>  2 files changed, 104 insertions(+), 29 deletions(-)

To be completely frank: If there is any drawback to using
SocketAddressFlat instead of SocketAdress, I would be against using it.
Less indirections in C is an argument, but less {} on the wire is not,
in my opinion. QMP is a machine interface and machines don't care about
that additional level.

Therefore, if you think this somehow affects compatibility, I would not
want to do this change.

I personally agree with Paolo that we can just break what we have, and
in addition I think that we *should* break what we have or we should not
do this change at all.

Max


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

Re: [Qemu-devel] [for-2.9 7/8] nbd: Tidy up blockdev-add interface

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 29.03.2017 18:45, 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, but
>> it's new in 2.9.  Replace it by SocketAddressFlat while we can.  This
>> simplifies
>> 
>>     { "execute": "blockdev-add",
>>       "arguments": { "node-name": "foo", "driver": "nbd",
>>                      "server": { "type": "inet",
>> 		                 "data": { "host": "localhost",
>> 				           "port": "12345" } } } }
>> 
>> to
>> 
>>     { "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          | 131 ++++++++++++++++++++++++++++++++++++++++-----------
>>  qapi/block-core.json |   2 +-
>>  2 files changed, 104 insertions(+), 29 deletions(-)
>
> To be completely frank: If there is any drawback to using
> SocketAddressFlat instead of SocketAdress, I would be against using it.
> Less indirections in C is an argument, but less {} on the wire is not,
> in my opinion. QMP is a machine interface and machines don't care about
> that additional level.
>
> Therefore, if you think this somehow affects compatibility, I would not
> want to do this change.

PRO:

1. Less nesting on the wire

   You think this isn't a valid argument.  I think it is, just a weak
   one.

2. Less indirection in C

   Makes dealing with the data type annoying.  In other words, the C
   interface is poor.  Poor C interfaces hiding in generated code is one
   thing, but SocketAddress is used in C interfaces between handwritten
   code.  Needs to go.

   Once its gone, all uses of SocketAddress we can't change for
   compatibility reasons need to go through a conversion function.  No
   biggie, but still, the fewer we have, the better.

3. Consistency within blockdev-add

   Nowhere else in blockdev-add do we wrap parts of socket addresses in
   "data".  I guess I should mention this in the commit message.

CON:

1. Renames -drive server.data.* to server.*.  These are new in 2.8.
   Libvirt doesn't use them.  I doubt anyone does.  My patch adds
   compatibility gunk anyway.

> I personally agree with Paolo that we can just break what we have, and
> in addition I think that we *should* break what we have or we should not
> do this change at all.

I'm totally fine with ripping out the compatibility gunk.

I put it in v1 so reviewers can like it or shoot it down.  Better than
not having it in v1, and have reviewers demand it for v2 just so they
can decide whether they want it.

Re: [Qemu-devel] [for-2.9 7/8] nbd: Tidy up blockdev-add interface

[Eric Blake]

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

On 03/29/2017 11:45 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, but
> it's new in 2.9.  Replace it by SocketAddressFlat while we can.  This
> simplifies
> 
>     { "execute": "blockdev-add",
>       "arguments": { "node-name": "foo", "driver": "nbd",
>                      "server": { "type": "inet",
> 		                 "data": { "host": "localhost",
> 				           "port": "12345" } } } }
> 
> to
> 
>     { "execute": "blockdev-add",
>       "arguments": { "node-name": "foo", "driver": "nbd",
>                      "server": { "type": "inet",
> 		                 "host": "localhost", "port": "12345" } } }

This part makes sense, and means this has to go in 2.9.

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

This is probably the fastest way forward for 2.9, even if we rip it out
later in 2.10.

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

Here, I'm 50/50 on whether the compatibility gunk is worth it, or
whether to make a clean break of old configurations, as I'm not sure who
is using the old configurations.  But given the lateness of the change
(and my recent case of being burned on qom-type during freeze when I was
not conservative), I agree with your conservative approach of remaining
compatible, even if it means the patch is larger than desired, and even
if we rip it out again in 2.10.

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

> @@ -223,17 +224,45 @@ 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 = qemu_opt_get(legacy_opts, "server.data.path");
> +    const char *sd_host = qemu_opt_get(legacy_opts, "server.data.host");
> +    const char *sd_port = qemu_opt_get(legacy_opts, "server.data.port");
> +    bool no_server = path || host || port;
> +    bool server_data = sd_path || sd_host || sd_port;
>      const QDictEntry *e;
>  
> -    if (!path && !host && !port) {
> +    if (!no_server && !server_data) {

Feels like a double negative, although it's not really serving that
role.  Maybe a better name would be s/no_server/bare/, so that 'bare' is
now your counterpart to 'server_data'.

> +        return true;
> +    }
> +
> +    if (no_server && server_data) {
> +        error_setg(errp, "Cannot use %s and %s at the same time",
> +                   "path/host/port", "server.data.*");
> +        return false;

Again, 'bare' makes this conditional read a bit better.

> +    }
> +
> +    if (server_data) {
> +        if (sd_host) {
> +            qdict_put(output_options, "server.host",
> +                      qstring_from_str(sd_host));
> +        }
> +        if (sd_port) {
> +            qdict_put(output_options, "server.port",
> +                      qstring_from_str(sd_port));
> +        }
> +        if (sd_path) {
> +            qdict_put(output_options, "server.path",
> +                      qstring_from_str(sd_path));
> +        }

Wow, I need to rebase my qdict_put_str() stuff again - that ought to be
something we include early in 2.10, as it touches a lot of stuff.
Doesn't affect your patch for now, though.

Renaming the local variable is trivial, so whether or not you
incorporate my naming suggestion,
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] [for-2.9 7/8] nbd: Tidy up blockdev-add interface

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/29/2017 11:45 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, but
>> it's new in 2.9.  Replace it by SocketAddressFlat while we can.  This
>> simplifies
>> 
>>     { "execute": "blockdev-add",
>>       "arguments": { "node-name": "foo", "driver": "nbd",
>>                      "server": { "type": "inet",
>> 		                 "data": { "host": "localhost",
>> 				           "port": "12345" } } } }
>> 
>> to
>> 
>>     { "execute": "blockdev-add",
>>       "arguments": { "node-name": "foo", "driver": "nbd",
>>                      "server": { "type": "inet",
>> 		                 "host": "localhost", "port": "12345" } } }
>
> This part makes sense, and means this has to go in 2.9.
>
>> 
>> Since the internal interfaces still take SocketAddress, this requires
>> conversion function socket_address_crumple().  It'll go away when I
>> update the interfaces.
>
> This is probably the fastest way forward for 2.9, even if we rip it out
> later in 2.10.
>
>> 
>> 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
>
> Here, I'm 50/50 on whether the compatibility gunk is worth it, or
> whether to make a clean break of old configurations, as I'm not sure who
> is using the old configurations.  But given the lateness of the change
> (and my recent case of being burned on qom-type during freeze when I was
> not conservative), I agree with your conservative approach of remaining
> compatible, even if it means the patch is larger than desired, and even
> if we rip it out again in 2.10.

Ripping out becomes harder the long we wait.

Since both Paolo and Max prefer to go without the compatibility gunk,
I'll drop it.

>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/nbd.c          | 131 ++++++++++++++++++++++++++++++++++++++++-----------
>>  qapi/block-core.json |   2 +-
>>  2 files changed, 104 insertions(+), 29 deletions(-)
>> 
>
>> @@ -223,17 +224,45 @@ 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 = qemu_opt_get(legacy_opts, "server.data.path");
>> +    const char *sd_host = qemu_opt_get(legacy_opts, "server.data.host");
>> +    const char *sd_port = qemu_opt_get(legacy_opts, "server.data.port");
>> +    bool no_server = path || host || port;
>> +    bool server_data = sd_path || sd_host || sd_port;
>>      const QDictEntry *e;
>>  
>> -    if (!path && !host && !port) {
>> +    if (!no_server && !server_data) {
>
> Feels like a double negative, although it's not really serving that
> role.  Maybe a better name would be s/no_server/bare/, so that 'bare' is
> now your counterpart to 'server_data'.

If we change our mind and keep the gunk, I'll rename to @bare.

>> +        return true;
>> +    }
>> +
>> +    if (no_server && server_data) {
>> +        error_setg(errp, "Cannot use %s and %s at the same time",
>> +                   "path/host/port", "server.data.*");
>> +        return false;
>
> Again, 'bare' makes this conditional read a bit better.
>
>> +    }
>> +
>> +    if (server_data) {
>> +        if (sd_host) {
>> +            qdict_put(output_options, "server.host",
>> +                      qstring_from_str(sd_host));
>> +        }
>> +        if (sd_port) {
>> +            qdict_put(output_options, "server.port",
>> +                      qstring_from_str(sd_port));
>> +        }
>> +        if (sd_path) {
>> +            qdict_put(output_options, "server.path",
>> +                      qstring_from_str(sd_path));
>> +        }
>
> Wow, I need to rebase my qdict_put_str() stuff again - that ought to be
> something we include early in 2.10, as it touches a lot of stuff.
> Doesn't affect your patch for now, though.
>
> Renaming the local variable is trivial, so whether or not you
> incorporate my naming suggestion,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Need to drop the R-by along with the compatibility gunk, but thanks
anyway!

[Qemu-devel] [for-2.9 8/8] 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.host=fido,vdi=dolly

instead of

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

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

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 89e98ed..60b9651 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1175,14 +1175,14 @@ 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);
     }
     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, "vdi", cfg.vdi);
     qdict_set_default_str(options, "tag", cfg.tag);
@@ -1510,15 +1510,15 @@ static QemuOptsList runtime_opts = {
     .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
     .desc = {
         {
-            .name = "host",
+            .name = "server.host",
             .type = QEMU_OPT_STRING,
         },
         {
-            .name = "port",
+            .name = "server.port",
             .type = QEMU_OPT_STRING,
         },
         {
-            .name = "path",
+            .name = "server.path",
             .type = QEMU_OPT_STRING,
         },
         {
@@ -1560,9 +1560,9 @@ 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");
+    host = qemu_opt_get(opts, "server.host");
+    port = qemu_opt_get(opts, "server.port");
+    path = qemu_opt_get(opts, "server.path");
     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);
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] [for-2.9 8/8] sheepdog: Fix blockdev-add

[Max Reitz]

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

On 29.03.2017 18:45, 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.host=fido,vdi=dolly
> 
> instead of
> 
>     --drive driver=sheepdog,host=fido,vdi=dolly
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c     | 18 +++++++++---------
>  qapi/block-core.json |  4 ++--
>  2 files changed, 11 insertions(+), 11 deletions(-)

OK, so let me get this straight: Before this patch, @addr was advertised
as a runtime parameter that actually wasn't supported at all? (Because I
can't see any place in block/sheepdog.c where it would be parsed.)

This patch now renames @addr to @server and instead of actually parsing
the option, it just removes the so-far convenience[1] options @host,
@port, and @path and just fetches the from @server?

If that is true, I can't say I like it the least. I guess it works for
2.9, but there should be a FIXME somewhere in this patch.

Also, if you set any of server.* in bdrv_parse_filename(), you also have
to set server.type. It's a SocketAddressFlat, .type is a required field.
I know it's just for internal use, but that doesn't change the fact that
it's an invalid SocketAddressFlat object without.

Max


[1] Originally there were apparently introduced for
bdrv_parse_filename() -- but of course you can just use them in -drive
directly...


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

Re: [Qemu-devel] [for-2.9 8/8] sheepdog: Fix blockdev-add

[Eric Blake]

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

On 03/29/2017 02:59 PM, Max Reitz wrote:
> On 29.03.2017 18:45, 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.host=fido,vdi=dolly
>>
>> instead of
>>
>>     --drive driver=sheepdog,host=fido,vdi=dolly
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/sheepdog.c     | 18 +++++++++---------
>>  qapi/block-core.json |  4 ++--
>>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> OK, so let me get this straight: Before this patch, @addr was advertised
> as a runtime parameter that actually wasn't supported at all? (Because I
> can't see any place in block/sheepdog.c where it would be parsed.)

Both commits mentioned in the commit message are new to 2.9, so we
aren't breaking any back-compat, but are avoiding cementing in a mistake.

Before this patch, @addr was the QMP spelling (inconsistent with all the
others, where ssh, gluster, and nbd spelled it @server); and because it
was NOT mentioned in the sheepdog.c QemuOpts code, it was impossible to
parse it through -drive.  Which means we have a needless difference
between -drive and -blockdev-add.

After this patch, it is spelled @server for both -drive and
-blockdev-add, and QMP is consistent (both with the QemuOpts code, and
with the other network storage drivers).

> 
> This patch now renames @addr to @server and instead of actually parsing
> the option, it just removes the so-far convenience[1] options @host,
> @port, and @path and just fetches the from @server?

But the convenience options have not been released, so fixing them now
is the right way to go.

> 
> If that is true, I can't say I like it the least. I guess it works for
> 2.9, but there should be a FIXME somewhere in this patch.

A fixme to what? Unlike patch 7/8 which has to deal with legacy -drive
code, here, we have nothing released to break.

> 
> Also, if you set any of server.* in bdrv_parse_filename(), you also have
> to set server.type. It's a SocketAddressFlat, .type is a required field.
> I know it's just for internal use, but that doesn't change the fact that
> it's an invalid SocketAddressFlat object without.

That part's true. Also, you can't set server.host and server.path at the
same time (compare to nbd_process_legacy_socket_options taking pains to
make sure that a valid QAPI object is constructed).

> 
> Max
> 
> 
> [1] Originally there were apparently introduced for
> bdrv_parse_filename() -- but of course you can just use them in -drive
> directly...

There's a difference between the psuedo-file '-drive sheepdog:...'
(where you can use them directly, and that doesn't change in this
patch), and the structured '-drive driver=sheepdog,...' (which didn't
exist before 831acdc, and which we want to match to our QMP).

I'm in favor of this patch, but think you found a real issue with not
constructing a valid qapi object.

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

[Max Reitz]

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

On 29.03.2017 22:32, Eric Blake wrote:
> On 03/29/2017 02:59 PM, Max Reitz wrote:
>> On 29.03.2017 18:45, 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.host=fido,vdi=dolly
>>>
>>> instead of
>>>
>>>     --drive driver=sheepdog,host=fido,vdi=dolly
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  block/sheepdog.c     | 18 +++++++++---------
>>>  qapi/block-core.json |  4 ++--
>>>  2 files changed, 11 insertions(+), 11 deletions(-)
>>
>> OK, so let me get this straight: Before this patch, @addr was advertised
>> as a runtime parameter that actually wasn't supported at all? (Because I
>> can't see any place in block/sheepdog.c where it would be parsed.)
> 
> Both commits mentioned in the commit message are new to 2.9, so we
> aren't breaking any back-compat, but are avoiding cementing in a mistake.
> 
> Before this patch, @addr was the QMP spelling (inconsistent with all the
> others, where ssh, gluster, and nbd spelled it @server); and because it
> was NOT mentioned in the sheepdog.c QemuOpts code, it was impossible to
> parse it through -drive.  Which means we have a needless difference
> between -drive and -blockdev-add.
> 
> After this patch, it is spelled @server for both -drive and
> -blockdev-add, and QMP is consistent (both with the QemuOpts code, and
> with the other network storage drivers).

Yes, right, which is why I agree that in principle this patch is
necessary for 2.9.

>> This patch now renames @addr to @server and instead of actually parsing
>> the option, it just removes the so-far convenience[1] options @host,
>> @port, and @path and just fetches the from @server?
> 
> But the convenience options have not been released, so fixing them now
> is the right way to go.

Right. My main point was that @server still isn't actually parsed.

>> If that is true, I can't say I like it the least. I guess it works for
>> 2.9, but there should be a FIXME somewhere in this patch.
> 
> A fixme to what? Unlike patch 7/8 which has to deal with legacy -drive
> code, here, we have nothing released to break.

A FIXME for parsing @server.

>> Also, if you set any of server.* in bdrv_parse_filename(), you also have
>> to set server.type. It's a SocketAddressFlat, .type is a required field.
>> I know it's just for internal use, but that doesn't change the fact that
>> it's an invalid SocketAddressFlat object without.
> 
> That part's true. Also, you can't set server.host and server.path at the
> same time (compare to nbd_process_legacy_socket_options taking pains to
> make sure that a valid QAPI object is constructed).
> 
>>
>> Max
>>
>>
>> [1] Originally there were apparently introduced for
>> bdrv_parse_filename() -- but of course you can just use them in -drive
>> directly...
> 
> There's a difference between the psuedo-file '-drive sheepdog:...'
> (where you can use them directly, and that doesn't change in this
> patch), and the structured '-drive driver=sheepdog,...' (which didn't
> exist before 831acdc, and which we want to match to our QMP).
> 
> I'm in favor of this patch, but think you found a real issue with not
> constructing a valid qapi object.

I agree that this patch is pretty much OK (and definitely necessary) for
2.9, what I'm asking for is some form of acknowledgment that we want to
actually make @server a valid SocketAddressFlat object and parse it as
such in 2.10.

Max


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

Re: [Qemu-devel] [for-2.9 8/8] sheepdog: Fix blockdev-add

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 29.03.2017 22:32, Eric Blake wrote:
>> On 03/29/2017 02:59 PM, Max Reitz wrote:
>>> On 29.03.2017 18:45, 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.host=fido,vdi=dolly
>>>>
>>>> instead of
>>>>
>>>>     --drive driver=sheepdog,host=fido,vdi=dolly
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>  block/sheepdog.c     | 18 +++++++++---------
>>>>  qapi/block-core.json |  4 ++--
>>>>  2 files changed, 11 insertions(+), 11 deletions(-)
>>>
>>> OK, so let me get this straight: Before this patch, @addr was advertised
>>> as a runtime parameter that actually wasn't supported at all? (Because I
>>> can't see any place in block/sheepdog.c where it would be parsed.)

Correct.  I messed it up.

>> Both commits mentioned in the commit message are new to 2.9, so we
>> aren't breaking any back-compat, but are avoiding cementing in a mistake.
>> 
>> Before this patch, @addr was the QMP spelling (inconsistent with all the
>> others, where ssh, gluster, and nbd spelled it @server); and because it
>> was NOT mentioned in the sheepdog.c QemuOpts code, it was impossible to
>> parse it through -drive.  Which means we have a needless difference
>> between -drive and -blockdev-add.

The difference being that -drive actually worked %-}

>> After this patch, it is spelled @server for both -drive and
>> -blockdev-add, and QMP is consistent (both with the QemuOpts code, and
>> with the other network storage drivers).
>
> Yes, right, which is why I agree that in principle this patch is
> necessary for 2.9.
>
>>> This patch now renames @addr to @server and instead of actually parsing
>>> the option, it just removes the so-far convenience[1] options @host,
>>> @port, and @path and just fetches the from @server?
>> 
>> But the convenience options have not been released, so fixing them now
>> is the right way to go.
>
> Right. My main point was that @server still isn't actually parsed.
>
>>> If that is true, I can't say I like it the least. I guess it works for
>>> 2.9, but there should be a FIXME somewhere in this patch.
>> 
>> A fixme to what? Unlike patch 7/8 which has to deal with legacy -drive
>> code, here, we have nothing released to break.
>
> A FIXME for parsing @server.

I'm not sure I understand what you mean by "parsing @server".  Care to
suggest a wording and a place for the FIXME comment?

I agree this code will need an update when we wean our internal
interfaces off SocketAddress.

>>> Also, if you set any of server.* in bdrv_parse_filename(), you also have
>>> to set server.type. It's a SocketAddressFlat, .type is a required field.
>>> I know it's just for internal use, but that doesn't change the fact that
>>> it's an invalid SocketAddressFlat object without.

Well, it's a flat QDict, not a SocketAddressFlat.  Is its "server.type"
member unset when nobody's looking?

Permit me to digress.  The block layer's QAPI/QDict/QemuOpts ménage à
trois is disgusting.  I understand how we got there, but that doesn't
make it less ugly.

Anyway, I'm willing to make bdrv_parse_filename() set "server.type".

Would you like me to require -drive server.type= when not using the
pseudo-filename, for consistency with other backends?

>> That part's true. Also, you can't set server.host and server.path at the
>> same time (compare to nbd_process_legacy_socket_options taking pains to
>> make sure that a valid QAPI object is constructed).

sd_open() rejects attempts to set both server.host and server.path.

>>> Max
>>>
>>>
>>> [1] Originally there were apparently introduced for
>>> bdrv_parse_filename() -- but of course you can just use them in -drive
>>> directly...
>> 
>> There's a difference between the psuedo-file '-drive sheepdog:...'
>> (where you can use them directly, and that doesn't change in this
>> patch), and the structured '-drive driver=sheepdog,...' (which didn't
>> exist before 831acdc, and which we want to match to our QMP).
>> 
>> I'm in favor of this patch, but think you found a real issue with not
>> constructing a valid qapi object.
>
> I agree that this patch is pretty much OK (and definitely necessary) for
> 2.9, what I'm asking for is some form of acknowledgment that we want to
> actually make @server a valid SocketAddressFlat object and parse it as
> such in 2.10.