Kevin Wolf <kw...@redhat.com> 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 <arm...@redhat.com>
>
>> +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
