Re: [Qemu-devel] [PATCH 15/27] rbd: Support .bdrv_co_create

From: Max Reitz <mreitz@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>, qemu-block@nongnu.org
Cc: pkrempa@redhat.com, eblake@redhat.com, jcody@redhat.com,
	jdurgin@redhat.com, mitake.hitoshi@lab.ntt.co.jp,
	namei.unix@gmail.com, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 15/27] rbd: Support .bdrv_co_create
Date: Mon, 12 Feb 2018 16:16:03 +0100	[thread overview]
Message-ID: <444b46ae-865a-ca6b-cacf-9898cf044891@redhat.com> (raw)
In-Reply-To: <20180208192328.16550-16-kwolf@redhat.com>

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

On 2018-02-08 20:23, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to rbd, which enables
> image creation over QMP.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/block-core.json |  20 +++++++-
>  block/rbd.c          | 137 +++++++++++++++++++++++++++++++++------------------
>  2 files changed, 108 insertions(+), 49 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Some comments below.

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5b4cd6bd12..370fcd9584 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3415,6 +3415,24 @@
>              '*refcount-bits':   'int' } }
>  
>  ##
> +# @BlockdevCreateOptionsRbd:
> +#
> +# Driver specific image creation options for rbd/Ceph.
> +#
> +# @location         Where to store the new image file

Maybe this should mention that location.snapshot is not allowed?

(And that location.server is ignored.  But is that even intended?)

> +# @size             Size of the virtual disk in bytes
> +# @password-secret  ID of secret providing the password
> +# @cluster_size     RBD object size

s/_/-/

> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateOptionsRbd',
> +  'data': { 'location':         'BlockdevOptionsRbd',
> +            'size':             'size',
> +            '*password-secret': 'str',
> +            '*cluster-size' :   'size' } }
> +
> +##
>  # @BlockdevCreateNotSupported:
>  #
>  # This is used for all drivers that don't support creating images.

[...]

> diff --git a/block/rbd.c b/block/rbd.c
> index a76a5e8755..c164f70167 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c

[...]

> @@ -432,24 +409,87 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)

[...]

> +static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
> +{
> +    BlockdevCreateOptions *create_options;
> +    BlockdevCreateOptionsRbd *rbd_opts;
> +    BlockdevOptionsRbd *loc;
> +    Error *local_err = NULL;
> +    const char *keypairs;
> +    QDict *options = NULL;
> +    int ret = 0;
> +
> +    create_options = g_new0(BlockdevCreateOptions, 1);
> +    create_options->driver = BLOCKDEV_DRIVER_RBD;
> +    rbd_opts = &create_options->u.rbd;
> +
> +    rbd_opts->location = g_new0(BlockdevOptionsRbd, 1);
> +
> +    rbd_opts->password_secret = (char *) qemu_opt_get(opts, "password-secret");
> +
> +    /* Read out options */
> +    rbd_opts->size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> +                              BDRV_SECTOR_SIZE);
> +    rbd_opts->cluster_size = qemu_opt_get_size_del(opts,
> +                                                   BLOCK_OPT_CLUSTER_SIZE, 0);
> +    rbd_opts->has_cluster_size = (rbd_opts->cluster_size != 0);
> +
> +    options = qdict_new();
> +    qemu_rbd_parse_filename(filename, options, &local_err);
> +    if (local_err) {
> +        ret = -EINVAL;
> +        error_propagate(errp, local_err);
> +        goto exit;
> +    }
> +
> +    /*
> +     * Caution: while qdict_get_try_str() is fine, getting non-string
> +     * types would require more care.  When @options come from -blockdev
> +     * or blockdev_add, its members are typed according to the QAPI
> +     * schema, but when they come from -drive, they're all QString.
> +     */
> +    loc = rbd_opts->location;
> +    loc->pool     = g_strdup(qdict_get_try_str(options, "pool"));
> +    loc->conf     = g_strdup(qdict_get_try_str(options, "conf"));
> +    loc->has_conf = !!rbd_opts->location->conf;
> +    loc->user     = g_strdup(qdict_get_try_str(options, "user"));
> +    loc->has_user = !!rbd_opts->location->user;

"!!loc->conf" and "!!loc->user" would be shorter and maybe a bit easier
to get.

Max

> +    loc->image    = g_strdup(qdict_get_try_str(options, "image"));
> +    keypairs      = qdict_get_try_str(options, "=keyvalue-pairs");
> +
> +    ret = qemu_rbd_do_create(create_options, keypairs, errp);
> +    if (ret < 0) {
> +        goto exit;
> +    }
>  
>  exit:
>      QDECREF(options);
> +    qapi_free_BlockdevCreateOptions(create_options);
>      return ret;
>  }

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]