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

[Eric Blake]

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 <address@hidden>
> ---
>  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 <address@hidden>

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

signature.asc
Description: OpenPGP digital signature