Re: [Qemu-block] [RFC v2 for-2.9 10/10] sheepdog: Fix blockdev-add

[Max Reitz]

On 30.03.2017 15:15, 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.type=inet,server.host=fido,server.port=7000,vdi=dolly
> 
> instead of
> 
>     --drive driver=sheepdog,host=fido,vdi=dolly
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  block/sheepdog.c     | 84 
> ++++++++++++++++++++++++++++++++++------------------
>  qapi/block-core.json |  4 +--
>  2 files changed, 58 insertions(+), 30 deletions(-)

There's trailing whitespace somewhere in this patch (git-am complained).

> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 89e98ed..c81013d 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -13,9 +13,11 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qapi-visit.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qint.h"
> +#include "qapi/qobject-input-visitor.h"
>  #include "qemu/uri.h"
>  #include "qemu/error-report.h"
>  #include "qemu/sockets.h"
> @@ -547,6 +549,47 @@ static SocketAddress *sd_socket_address(const char *path,
>      return addr;
>  }
>  
> +static SocketAddress *sd_server_config(QDict *options, Error **errp)
> +{
> +    QDict *server = NULL;
> +    QObject *crumpled_server = NULL;
> +    Visitor *iv = NULL;
> +    SocketAddressFlat *saddr_flat = NULL;
> +    SocketAddress *saddr = NULL;
> +    Error *local_err = NULL;
> +
> +    qdict_extract_subqdict(options, &server, "server.");

server may be empty:

$ ./qemu-img info sheepdog:vdi
qemu-img: Could not open 'sheepdog:vdi': Parameter 'type' is missing

I'd propose creating an 'inet' SocketAddress object then for
SD_DEFAULT_ADDR and SD_DEFAULT_PORT so that existing behavior won't change.

> +
> +    crumpled_server = qdict_crumple(server, errp);
> +    if (!crumpled_server) {
> +        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_server);
> +    visit_type_SocketAddressFlat(iv, NULL, &saddr_flat, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto done;
> +    }
> +
> +    saddr = socket_address_crumple(saddr_flat);
> +
> +done:
> +    qapi_free_SocketAddressFlat(saddr_flat);
> +    visit_free(iv);
> +    qobject_decref(crumpled_server);
> +    QDECREF(server);
> +    return saddr;
> +}
> +
>  /* Return -EIO in case of error, file descriptor on success */
>  static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
>  {
> @@ -1175,15 +1218,17 @@ 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);
> +        qdict_set_default_str(options, "server.type", "inet");
>      }
>      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);

.port is mandatory, so doing this optionally results in:

$ ./qemu-img info sheepdog://localhost/foo
qemu-img: Could not open 'sheepdog://localhost/foo': Parameter 'port' is
missing

>      }
>      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, "server.type", "unix");
> +   }

Indentation is off.

Max

>      qdict_set_default_str(options, "vdi", cfg.vdi);
>      qdict_set_default_str(options, "tag", cfg.tag);
>      if (cfg.snap_id) {
> @@ -1510,18 +1555,6 @@ static QemuOptsList runtime_opts = {
>      .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
>      .desc = {
>          {
> -            .name = "host",
> -            .type = QEMU_OPT_STRING,
> -        },
> -        {
> -            .name = "port",
> -            .type = QEMU_OPT_STRING,
> -        },
> -        {
> -            .name = "path",
> -            .type = QEMU_OPT_STRING,
> -        },
> -        {
>              .name = "vdi",
>              .type = QEMU_OPT_STRING,
>          },
> @@ -1543,7 +1576,7 @@ static int sd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      int ret, fd;
>      uint32_t vid = 0;
>      BDRVSheepdogState *s = bs->opaque;
> -    const char *host, *port, *path, *vdi, *snap_id_str, *tag;
> +    const char *vdi, *snap_id_str, *tag;
>      uint64_t snap_id;
>      char *buf = NULL;
>      QemuOpts *opts;
> @@ -1560,20 +1593,17 @@ 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");
> +    s->addr = sd_server_config(options, errp);
> +    if (!s->addr) {
> +        ret = -EINVAL;
> +        goto err_no_fd;
> +    }
> +     
>      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);
>      tag = qemu_opt_get(opts, "tag");
>  
> -    if ((host || port) && path) {
> -        error_setg(errp, "can't use 'path' together with 'host' or 'port'");
> -        ret = -EINVAL;
> -        goto err_no_fd;
> -    }
> -
>      if (!vdi) {
>          error_setg(errp, "parameter 'vdi' is missing");
>          ret = -EINVAL;
> @@ -1604,8 +1634,6 @@ static int sd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>          goto err_no_fd;
>      }
>  
> -    s->addr = sd_socket_address(path, host, port);
> -
>      QLIST_INIT(&s->inflight_aio_head);
>      QLIST_INIT(&s->failed_aio_head);
>      QLIST_INIT(&s->inflight_aiocb_head);
> 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' } }
>

signature.asc
Description: OpenPGP digital signature