Re: [Qemu-devel] [PATCH v3 08/14] block/nbd: Accept SocketAddress

[Eric Blake <eblake@redhat.com>]

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

On 04/06/2016 12:28 PM, Max Reitz wrote:
> Add a new option "address" to the NBD block driver which accepts a
> SocketAddress.
> 
> "path", "host" and "port" are still supported as legacy options and are
> mapped to their corresponding SocketAddress representation.

The back-compat work is pretty slick.

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/nbd.c                   | 97 ++++++++++++++++++++++++++-----------------
>  tests/qemu-iotests/051.out    |  4 +-
>  tests/qemu-iotests/051.pc.out |  4 +-
>  3 files changed, 64 insertions(+), 41 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 3adf302..9ae66ba 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -32,6 +32,8 @@
>  #include "qemu/uri.h"
>  #include "block/block_int.h"
>  #include "qemu/module.h"
> +#include "qapi-visit.h"
> +#include "qapi/qmp-input-visitor.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qjson.h"
>  #include "qapi/qmp/qint.h"
> @@ -128,7 +130,9 @@ static bool nbd_has_filename_options_conflict(QDict *options, Error **errp)
>          if (!strcmp(e->key, "host")
>              || !strcmp(e->key, "port")
>              || !strcmp(e->key, "path")
> -            || !strcmp(e->key, "export"))
> +            || !strcmp(e->key, "export")
> +            || !strcmp(e->key, "address")
> +            || !strncmp(e->key, "address.", 8))

May need a tweak if you break before '||'

>          {
>              error_setg(errp, "Option '%s' cannot be used with a file name",
>                         e->key);
> @@ -202,46 +206,66 @@ out:
>      g_free(file);
>  }
>  
> +static bool nbd_process_legacy_socket_options(QDict *options, Error **errp)
> +{
> +    if (qdict_haskey(options, "path") && qdict_haskey(options, "host")) {
> +        error_setg(errp, "path and host may not be used at the same time");
> +        return false;
> +    } else if (qdict_haskey(options, "path")) {
> +        if (qdict_haskey(options, "port")) {
> +            error_setg(errp, "port may not be used without host");
> +            return false;
> +        }
> +
> +        qdict_put(options, "address.type", qstring_from_str("unix"));
> +        qdict_change_key(options, "path", "address.data.path");
> +    } else if (qdict_haskey(options, "host")) {
> +        qdict_put(options, "address.type", qstring_from_str("inet"));
> +        qdict_change_key(options, "host", "address.data.host");
> +        if (!qdict_change_key(options, "port", "address.data.port")) {
> +            qdict_put(options, "address.data.port",
> +                      qstring_from_str(stringify(NBD_DEFAULT_PORT)));
> +        }

The rewrite from old to modern is rather nice.  I wonder if we could
then use a generated qapi_visit_SocketAddress instead of manually
handling all the complication of SocketAddress proper.

> +    }
> +
> +    return true;
> +}
> +
>  static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
>                                   Error **errp)
>  {
> -    SocketAddress *saddr;
> +    SocketAddress *saddr = NULL;
> +    QDict *addr = NULL;
> +    QObject *crumpled_addr;
> +    QmpInputVisitor *iv;
> +    Error *local_err = NULL;
>  
> -    if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
> -        if (qdict_haskey(options, "path")) {
> -            error_setg(errp, "path and host may not be used at the same time");
> -        } else {
> -            error_setg(errp, "one of path and host must be specified");
> -        }
> -        return NULL;
> +    if (!nbd_process_legacy_socket_options(options, errp)) {
> +        goto fail;
>      }
> -    if (qdict_haskey(options, "port") && !qdict_haskey(options, "host")) {
> -        error_setg(errp, "port may not be used without host");
> -        return NULL;
> +
> +    qdict_extract_subqdict(options, &addr, "address.");
> +    if (!qdict_size(addr)) {
> +        error_setg(errp, "NBD server address missing");
> +        goto fail;
>      }
>  
> -    saddr = g_new0(SocketAddress, 1);
> +    crumpled_addr = qdict_crumple(addr, true, errp);
> +    if (!crumpled_addr) {
> +        goto fail;
> +    }
>  

Ah, so you ARE depending on Dan's qdict_crumple().

> -    if (qdict_haskey(options, "path")) {
> -        UnixSocketAddress *q_unix;
> -        saddr->type = SOCKET_ADDRESS_KIND_UNIX;
> -        q_unix = saddr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
> -        q_unix->path = g_strdup(qdict_get_str(options, "path"));
> -        qdict_del(options, "path");
> -    } else {
> -        InetSocketAddress *inet;
> -        saddr->type = SOCKET_ADDRESS_KIND_INET;
> -        inet = saddr->u.inet.data = g_new0(InetSocketAddress, 1);
> -        inet->host = g_strdup(qdict_get_str(options, "host"));
> -        if (!qdict_get_try_str(options, "port")) {
> -            inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
> -        } else {
> -            inet->port = g_strdup(qdict_get_str(options, "port"));
> -        }
> -        qdict_del(options, "host");
> -        qdict_del(options, "port");
> +    iv = qmp_input_visitor_new(crumpled_addr);
> +    visit_type_SocketAddress(qmp_input_get_visitor(iv), NULL, &saddr,
> +                             &local_err);

and you DO use the generated QAPI code.  Cool!

> +    qmp_input_visitor_cleanup(iv);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto fail;
>      }
>  
> +    /* TODO: Detect superfluous (unused) options under in addr */

Is this TODO addressed anywhere, or mentioned in the commit message?

In fact, I think it's quite easy to address: qmp_input_visitor_new()
recently changed signatures (commit fc471c18), so all you have to do is
pass strict=true to that function as part of rebasing your work, at
which point you will automatically reject any leftover keys in addr that
didn't get parsed by SocketAddress.

Needs a rebase, but looks promising.

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