Re: [Qemu-devel] [RFC PATCH 06/19] block: Fix -blockdev for certain non-string scalars

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 07.06.2018 um 08:25 hat Markus Armbruster geschrieben:
>> Configuration flows through the block subsystem in a rather peculiar
>> way.  Configuration made with -drive enters it as QemuOpts.
>> Configuration made with -blockdev / blockdev-add enters it as QAPI
>> type BlockdevOptions.  The block subsystem uses QDict, QemuOpts and
>> QAPI types internally.  The precise flow is next to impossible to
>> explain (I tried for this commit message, but gave up after wasting
>> several hours).  What I can explain is a flaw in the BlockDriver
>> interface that leads to this bug:
>> 
>>     $ qemu-system-x86 -blockdev 
>> node-name=n1,driver=nfs,server.type=inet,server.host=localhost,path=/foo/bar,user=1234:
>>  Internal error: parameter user invalid
>>     qemu-system-x86: -blockdev 
>> node-name=n1,driver=nfs,server.type=inet,server.host=localhost,path=/foo/bar,user=1234:
>>  Internal error: parameter user invalid
>
> I don't think the error message was intended to be part of your command
> line, and I also don't have a binary called qemu-system-x86. :-)

Uh, I managed to combine a pasto with a typo.  Fixing...

>> QMP blockdev-add is broken the same way.
>> 
>> Here's what happens.  The block layer passes configuration represented
>> as flat QDict (with dotted keys) to BlockDriver methods
>> .bdrv_file_open().  The QDict's members are typed according to the
>> QAPI schema.
>> 
>> nfs_file_open() converts it to QAPI type BlockdevOptionsNfs, with
>> qdict_crumple() and a qobject input visitor.
>> 
>> This visitor comes in two flavors.  The plain flavor requires scalars
>> to be typed according to the QAPI schema.  That's the case here.  The
>> keyval flavor requires string scalars.  That's not the case here.
>> nfs_file_open() uses the latter, and promptly falls apart for members
>> @user, @group, @tcp-syn-count, @readahead-size, @page-cache-size,
>> @debug.
>> 
>> Switching to the plain flavor would fix -blockdev, but break -drive,
>> because there the scalars arrive in nfs_file_open() as strings.
>> 
>> The proper fix would be to replace the QDict by QAPI type
>> BlockdevOptions in the BlockDriver interface.  Sadly, that's beyond my
>> reach right now.
>> 
>> Next best would be to fix the block layer to always pass correctly
>> typed QDicts to the BlockDriver methods.  Also beyond my reach.
>> 
>> What I can do is throw another hack onto the pile: have
>> nfs_file_open() convert all members to string, so use of the keyval
>> flavor actually works, by replacing qdict_crumple() by new function
>> qdict_crumple_for_keyval_qiv().
>> 
>> The pattern "pass result of qdict_crumple() to
>> qobject_input_visitor_new_keyval()" occurs several times more:
>> 
>> * qemu_rbd_open()
>> 
>>   Same issue as nfs_file_open(), but since BlockdevOptionsRbd has only
>>   string members, its only a latent bug.  Fix it anyway.
>> 
>> * parallels_co_create_opts(), qcow_co_create_opts(),
>>   qcow2_co_create_opts(), bdrv_qed_co_create_opts(),
>>   sd_co_create_opts(), vhdx_co_create_opts(), vpc_co_create_opts()
>> 
>>   These work, because they create the QDict with
>>   qemu_opts_to_qdict_filtered(), which creates only string scalars.
>>   The function sports a TODO comment asking for better typing; that's
>>   going to be fun.  Use qdict_crumple_for_keyval_qiv() to be safe.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>
>> +QObject *qdict_crumple_for_keyval_qiv(QDict *src, Error **errp)
>> +{
>> +    QDict *tmp = NULL;
>> +    char *buf;
>> +    const char *s;
>> +    const QDictEntry *ent;
>> +    QObject *dst;
>> +
>> +    for (ent = qdict_first(src); ent; ent = qdict_next(src, ent)) {
>> +        buf = NULL;
>> +        switch (qobject_type(ent->value)) {
>> +        case QTYPE_QNULL:
>> +        case QTYPE_QSTRING:
>> +            continue;
>> +        case QTYPE_QNUM:
>> +            s = buf = qnum_to_string(qobject_to(QNum, ent->value));
>> +            break;
>> +        case QTYPE_QDICT:
>> +        case QTYPE_QLIST:
>> +            /* @src isn't flat; qdict_crumple() will fail */
>> +            continue;
>> +        case QTYPE_QBOOL:
>> +            s = qbool_get_bool(qobject_to(QBool, ent->value))
>> +                ? "on" : "off";
>
> This fits in a single line.

Pushing column 77, for an effective line width of 65 characters.  I hate
that :)

I could be persuaded to add qbool_to_string().

>> +            break;
>> +        default:
>> +            abort();
>> +        }
>> +
>> +        if (!tmp) {
>> +            tmp = qdict_clone_shallow(src);
>> +        }
>> +        qdict_put(tmp, ent->key, qstring_from_str(s));
>> +        g_free(buf);
>> +    }
>> +
>> +    dst = qdict_crumple(tmp ?: src, errp);
>> +    qobject_unref(tmp);
>> +    return dst;
>> +}
>
> Kevin