Re: [Qemu-devel] [PATCH v3 6/8] block: Try to create well typed json:{} filenames

[Eric Blake <eblake@redhat.com>]

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

On 2/6/19 1:55 PM, Max Reitz wrote:

In the subject, s/well typed/well-typed/

> By applying a health mix of qdict_flatten(), qdict_crumple(),

s/health/healty/

> qdict_stringify_for_keyval(), the keyval input visitor, and the QObject
> output visitor (not necessarily in that order), we can at least try to
> bring bs->full_open_options into accordance with the QAPI schema.  This
> may not always work (there are some options left that have not been
> QAPI-fied yet), but in practice it usually will.
> 
> In any case, sometimes emitting wrongly typed json:{} filenames is
> better than doing it effectively half the time.

And someday, if we ever switch the block layer to use QAPI types all the
way, we can drop some of these hacks that have built up over time. But
not a show-stopper for this series.

> 
> This affects some iotests because json:{} filenames are now usually
> crumpled.  In 198, "format": "auto" now appears in the qcow2 encryption
> options because going through a visitor makes optional discriminators'
> default values explicit.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1534396
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                    | 68 +++++++++++++++++++++++++++++++++++++-
>  tests/qemu-iotests/059.out |  2 +-
>  tests/qemu-iotests/099.out |  4 +--
>  tests/qemu-iotests/110.out |  2 +-
>  tests/qemu-iotests/198.out |  4 +--
>  tests/qemu-iotests/207.out | 10 +++---
>  6 files changed, 78 insertions(+), 12 deletions(-)
> 

> +/**
> + * Take a blockdev @options QDict and convert its values to the
> + * correct type.
> + *
> + * Fail if @options does not match the QAPI schema of BlockdevOptions.
> + *
> + * In case of failure, return NULL and set @errp.
> + *
> + * In case of success, return a correctly typed new QDict.
> + */
> +static QDict *bdrv_type_blockdev_opts(const QDict *options, Error **errp)
> +{
> +    Visitor *v;
> +    BlockdevOptions *blockdev_options;
> +    QObject *typed_opts;
> +    QDict *string_options;
> +    Error *local_err = NULL;
> +
> +    string_options = qdict_clone_shallow(options);
> +
> +    qdict_flatten(string_options);
> +    v = qobject_input_visitor_new_flat_confused(string_options, errp);
> +    if (!v) {
> +        error_prepend(errp, "Failed to prepare options: ");
> +        return NULL;
> +    }
> +
> +    visit_type_BlockdevOptions(v, NULL, &blockdev_options, &local_err);
> +    visit_free(v);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        error_prepend(errp, "Not a valid BlockdevOptions object: ");
> +        return NULL;
> +    }
> +
> +    v = qobject_output_visitor_new(&typed_opts);
> +    visit_type_BlockdevOptions(v, NULL, &blockdev_options, &local_err);
> +    if (!local_err) {
> +        visit_complete(v, &typed_opts);
> +    }
> +    visit_free(v);
> +    qapi_free_BlockdevOptions(blockdev_options);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return NULL;
> +    }
> +
> +    return qobject_to(QDict, typed_opts);
> +}
> +
>  /* Updates the following BDS fields:
>   *  - exact_filename: A filename which may be used for opening a block device
>   *                    which (mostly) equals the given BDS (even without any
> @@ -5698,10 +5749,25 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>      if (bs->exact_filename[0]) {
>          pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
>      } else {
> -        QString *json = qobject_to_json(QOBJECT(bs->full_open_options));
> +        QString *json;
> +        QDict *typed_opts, *json_opts;
> +
> +        typed_opts = bdrv_type_blockdev_opts(bs->full_open_options, NULL);
> +
> +        /*
> +         * We cannot be certain that bs->full_open_options matches
> +         * BlockdevOptions, so bdrv_type_blockdev_opts() may fail.
> +         * That is not fatal, we can just emit bs->full_open_options
> +         * directly -- qemu will accept that, even if it does not
> +         * match the schema.
> +         */
> +        json_opts = typed_opts ?: bs->full_open_options;
> +
> +        json = qobject_to_json(QOBJECT(json_opts));
>          snprintf(bs->filename, sizeof(bs->filename), "json:%s",
>                   qstring_get_str(json));

I so need to revive my series that created a JSON output visitor (to
skip the qobject_to_json() intermediate step). But that's a topic for
another day.

> +++ b/tests/qemu-iotests/059.out
> @@ -2050,7 +2050,7 @@ wrote 512/512 bytes at offset 10240
>  
>  === Testing monolithicFlat with internally generated JSON file name ===
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 subformat=monolithicFlat
> -can't open: Cannot use relative extent paths with VMDK descriptor file 'json:{"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "inject-error.0.event": "read_aio"}'
> +can't open: Cannot use relative extent paths with VMDK descriptor file 'json:{"inject-error": [{"event": "read_aio"}], "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug"}'

"inject-error" sorts after "image"; meanwhile, the QAPI definition of
BlockdevOptionsBlkdebug sorts image before inject-error. It looks like
this output is randomized; can that bite us (as we've recently found
with other iotests where python 2 vs. 3 sorting mattered)?  And can we
do better?  (My QAPI JSON output visitor would guarantee the output in
QAPI definition order)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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