Re: [Qemu-devel] [PATCH v4 06/12] block/nbd: Accept SocketAddress

[Kevin Wolf]

Am 28.09.2016 um 22:55 hat Max Reitz geschrieben:
> 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.
> 
> Signed-off-by: Max Reitz <address@hidden>

Not opposed in principle to your change, but we should try to keep the
naming consistent between NBD and the other block drivers, notably the
SSH work that is currently going on.

This patch uses 'address' for the SockAddress, the proposed SSH patch
uses 'server'. I don't mind too much which one we choose, though I like
'server' a bit better. Anyway, we should choose one and stick to it in
all drivers.

>  block/nbd.c                   | 166 
> ++++++++++++++++++++++++++----------------
>  tests/qemu-iotests/051.out    |   4 +-
>  tests/qemu-iotests/051.pc.out |   4 +-
>  3 files changed, 106 insertions(+), 68 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index cdab20f..449f94e 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -32,6 +32,9 @@
>  #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-output-visitor.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qjson.h"
>  #include "qapi/qmp/qint.h"
> @@ -44,7 +47,8 @@ typedef struct BDRVNBDState {
>      NbdClientSession client;
>  
>      /* For nbd_refresh_filename() */
> -    char *path, *host, *port, *export, *tlscredsid;
> +    SocketAddress *saddr;
> +    char *export, *tlscredsid;
>  } BDRVNBDState;
>  
>  static int nbd_parse_uri(const char *filename, QDict *options)
> @@ -131,7 +135,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))

strstart()?

>          {
>              error_setg(errp, "Option '%s' cannot be used with a file name",
>                         e->key);
> @@ -205,50 +211,67 @@ out:
>      g_free(file);
>  }
>  
> -static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Error 
> **errp)
> +static bool nbd_process_legacy_socket_options(QDict *output_options,
> +                                              QemuOpts *legacy_opts,
> +                                              Error **errp)
>  {
> -    SocketAddress *saddr;
> -
> -    s->path = g_strdup(qemu_opt_get(opts, "path"));
> -    s->host = g_strdup(qemu_opt_get(opts, "host"));
> -    s->port = g_strdup(qemu_opt_get(opts, "port"));
> -
> -    if (!s->path == !s->host) {
> -        if (s->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");
> +    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");
> +
> +    if (path && host) {
> +        error_setg(errp, "path and host may not be used at the same time");
> +        return false;
> +    } else if (path) {
> +        if (port) {
> +            error_setg(errp, "port may not be used without host");
> +            return false;
>          }
> -        return NULL;
> +
> +        qdict_put(output_options, "address.type", qstring_from_str("unix"));
> +        qdict_put(output_options, "address.data.path", 
> qstring_from_str(path));
> +    } else if (host) {
> +        qdict_put(output_options, "address.type", qstring_from_str("inet"));
> +        qdict_put(output_options, "address.data.host", 
> qstring_from_str(host));
> +        qdict_put(output_options, "address.data.port",
> +                  qstring_from_str(port ?: stringify(NBD_DEFAULT_PORT)));
>      }
> -    if (s->port && !s->host) {
> -        error_setg(errp, "port may not be used without host");
> -        return NULL;
> +
> +    return true;
> +}

If both the legacy option and the new one are given, the legacy one
takes precedence. Intentional?

Kevin