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

[Max Reitz <mreitz@redhat.com>]

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

On 06.02.19 21:43, Eric Blake wrote:
> 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),

More importantly, this is just wrong (or rather, outdated).  I fixed it
in the cover letter but forgot to fix it here.  This version just uses
qdict_flatten(), the flat-confused input visitor and the output visitor.
 There is no longer a qdict_stringify_for_keyval() because Markus's
flat-confused visitor made it unnecessary.

>>                                                 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)

It can't bite us in the Python 2 vs. 3 sense because the order is
determined by qemu code alone (so it is stable for one version of qemu),
but of course a real order would be nicer.

I don't think it makes sense to parse the filename in the iotest (for
one thing, we'd need to convert this test to Python for that) so we'd be
able to guarantee some order in the output, so I think it's best to just
live with it for now.

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

Once again, thanks!

Max


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