Re: [Qemu-block] [PATCH 10/27] qcow2: Use visitor for options in qcow2_create()

[Eric Blake]

On 02/08/2018 01:23 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>   block/qcow2.c              | 219 ++++++++++++++++-----------------------------
>   tests/qemu-iotests/049.out |   8 +-
>   tests/qemu-iotests/112.out |   4 +-
>   3 files changed, 84 insertions(+), 147 deletions(-)

>       BlockDriverState *bs = NULL;
> -    Error *local_err = NULL;
> +    const char *val;
>       int ret;
> +    Error *local_err = NULL;
>   

Worth the churn on the local_err declaration position?

> -    /* Read out options */
> -    size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> -                    BDRV_SECTOR_SIZE);
> -    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> -    backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
> -    backing_drv = qapi_enum_parse(&BlockdevDriver_lookup, backing_fmt,
> -                                  0, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    /* Only the keyval visitor supports the dotted syntax needed for
> +     * encryption, so go through a QDict before getting a QAPI type. Ignore
> +     * options meant for the protocol layer so that the visitor doesn't
> +     * complain. */
> +    qdict = qemu_opts_to_qdict_filtered(opts, NULL, bdrv_qcow2.create_opts,
> +                                        true);

Glue code at its finest ;)

> +    /* Convert compat=0.10/1.1 into compat=v2/v3, to be renamed into
> +     * version=v2/v3 below. */
> +    val = qdict_get_try_str(qdict, BLOCK_OPT_COMPAT_LEVEL);
> +    if (val && !strcmp(val, "0.10")) {
> +        qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "v2");
> +    } else if (val && !strcmp(val, "1.1")) {
> +        qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "v3");
> +    }

Not only does this map the old 'qemu-img create -o' spellings into the 
QMP form, but it means that we now also accept the new spelling via 
qemu-img command line.  Might be worth mentioning in the commit message 
as an intentional enhancement.

> +
> +    /* Change legacy command line options into QMP ones */
> +    static const QDictRenames opt_renames[] = {
> +        { BLOCK_OPT_BACKING_FILE,       "backing-file" },
> +        { BLOCK_OPT_BACKING_FMT,        "backing-fmt" },
> +        { BLOCK_OPT_CLUSTER_SIZE,       "cluster-size" },
> +        { BLOCK_OPT_LAZY_REFCOUNTS,     "lazy-refcounts" },
> +        { BLOCK_OPT_REFCOUNT_BITS,      "refcount-bits" },
> +        { BLOCK_OPT_ENCRYPT,            BLOCK_OPT_ENCRYPT_FORMAT },
> +        { BLOCK_OPT_COMPAT_LEVEL,       "version" },
> +        { NULL, NULL },
> +    };

Looks reasonable to me.

> -
> -    cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        ret = -EINVAL;
> +    /* Create and open the file (protocol layer) */
> +    ret = bdrv_create_file(filename, opts, errp);
> +    if (ret < 0) {
>           goto finish;

Git got lost on producing a sane diff.  Oh well.

> -    version = qcow2_opt_get_version_del(opts, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    /* Set 'driver' and 'node' options */
> +    qdict_put_str(qdict, "driver", "qcow2");
> +    qdict_put_str(qdict, "file", bs->node_name);
> +
> +    /* Now get the QAPI type BlockdevCreateOptions */
> +    qobj = qdict_crumple(qdict, errp);
> +    QDECREF(qdict);
> +    qdict = qobject_to_qdict(qobj);
> +    if (qdict == NULL) {

Fun with round trips.  Maybe someday we can improve things, but for now, 
I'm glad that it at least works.

>           ret = -EINVAL;
>           goto finish;
>       }
>   
> -    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_LAZY_REFCOUNTS, false)) {
> -        flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
> -    }
> +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
> +    visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
> +    visit_free(v);

But this part is definitely the payout of what we wanted to get to!


>       /* Create the qcow2 image (format layer) */
> -    create_options = (BlockdevCreateOptions) {
> -        .driver         = BLOCKDEV_DRIVER_QCOW2,
> -        .u.qcow2        = {
> -            .file               = &(BlockdevRef) {

And using the visitor is a lot nicer than populating the struct by hand.

> +++ b/tests/qemu-iotests/049.out
> @@ -166,11 +166,11 @@ qemu-img create -f qcow2 -o compat=1.1 TEST_DIR/t.qcow2 
> 64M
>   Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=1.1 
> cluster_size=65536 lazy_refcounts=off refcount_bits=16
>   
>   qemu-img create -f qcow2 -o compat=0.42 TEST_DIR/t.qcow2 64M
> -qemu-img: TEST_DIR/t.qcow2: Invalid compatibility level: '0.42'
> +qemu-img: TEST_DIR/t.qcow2: Invalid parameter '0.42'
>   Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=0.42 
> cluster_size=65536 lazy_refcounts=off refcount_bits=16

Yep, the visitor has slightly different messages, but I'm fine with the 
fallout.

Reviewed-by: Eric Blake <address@hidden>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org