Re: [Qemu-devel] [PATCH v3 07/14] block/nbd: "address" in nbd_refresh_filename()

[Eric Blake <eblake@redhat.com>]

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

On 04/06/2016 12:28 PM, Max Reitz wrote:
> As of a future patch, the NBD block driver will accept a SocketAddress
> structure for a new "address" option. In order to support this,
> nbd_refresh_filename() needs some changes.
> 
> The two TODOs introduced by this patch will be removed in the very next
> one. They exist to explain that it is currently impossible for
> nbd_refresh_filename() to emit an "address.*" option (which the NBD
> block driver does not handle yet). The next patch will arm these code
> paths, but it will also enable handling of these options.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/nbd.c | 84 ++++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 61 insertions(+), 23 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 1736f68..3adf302 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -441,37 +441,75 @@ static void nbd_attach_aio_context(BlockDriverState *bs,
>  static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
>  {
>      QDict *opts = qdict_new();
> -    const char *path   = qdict_get_try_str(options, "path");
> -    const char *host   = qdict_get_try_str(options, "host");
> -    const char *port   = qdict_get_try_str(options, "port");
> +    bool can_generate_filename = true;
> +    const char *path = NULL, *host = NULL, *port = NULL;
>      const char *export = qdict_get_try_str(options, "export");
>      const char *tlscreds = qdict_get_try_str(options, "tls-creds");
>  
> -    if (host && !port) {
> -        port = stringify(NBD_DEFAULT_PORT);
> +    if (qdict_get_try_str(options, "address.type")) {
> +        /* This path will only be possible as of a future patch;
> +         * TODO: Remove this note once it is */
> +
> +        const char *type = qdict_get_str(options, "address.type");
> +

Oh, I'm sooooo tempted to teach the QAPI generator how to make a
discriminated union have a default 'type' value (thus making the
discriminator optional), so that we don't need a layer of nesting behind
'address.'.

> +        if (!strcmp(type, "unix")) {
> +            path = qdict_get_str(options, "address.data.path");
> +        } else if (!strcmp(type, "inet")) {
> +            host = qdict_get_str(options, "address.data.host");
> +            port = qdict_get_str(options, "address.data.port");

It's especially annoying that because SocketAddress is not flat, we have
to expose the 'data.' layer of nesting, even if we could avoid the
'address.' layer.

> +
> +            can_generate_filename = !qdict_haskey(options, "address.data.to")
> +                                 && !qdict_haskey(options, "address.data.ipv4")
> +                                 && !qdict_haskey(options, "address.data.ipv6");
> +        } else {
> +            can_generate_filename = false;
> +        }
> +    } else {
> +        path = qdict_get_try_str(options, "path");
> +        host = qdict_get_try_str(options, "host");
> +        port = qdict_get_try_str(options, "port");
> +
> +        if (host && !port) {
> +            port = stringify(NBD_DEFAULT_PORT);
> +        }
>      }

Looks clean given the constraints of what you are able to use from QAPI.

> +
> +    if (qdict_get_try_str(options, "address.type")) {
> +        /* This path will only be possible as of a future patch;
> +         * TODO: Remove this note once it is */
> +
> +        const QDictEntry *e;
> +        for (e = qdict_first(options); e; e = qdict_next(options, e)) {
> +            if (!strncmp(e->key, "address.", 8)) {
> +                qobject_incref(e->value);
> +                qdict_put_obj(opts, e->key, e->value);
> +            }
> +        }

This part makes me wonder if we want Dan's qdict_crumple() working first.

>      } else {
> -        qdict_put(opts, "host", qstring_from_str(host));
> -        qdict_put(opts, "port", qstring_from_str(port));
> +        if (path) {
> +            qdict_put(opts, "path", qstring_from_str(path));
> +        } else {
> +            qdict_put(opts, "host", qstring_from_str(host));
> +            qdict_put(opts, "port", qstring_from_str(port));
> +        }
>      }
>      if (export) {
>          qdict_put(opts, "export", qstring_from_str(export));
> 

At this point, I'll reserve giving R-b until I've seen the whole series
(it may need rebasing anyways...)

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