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

[Kevin Wolf <kwolf@redhat.com>]

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

> 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 <armbru@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.

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