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

[Max Reitz]

On 2018-06-07 08:25, Markus Armbruster wrote:
> 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
> 
> 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.

(I'll just chime in because why not.)

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

Agreed.  The way there probably is to (as always) unify how the block
drivers convert their QDict to their own BlockdevOptions, and then pull
that out into block.c, but let's see how far off that still is.

> Next best would be to fix the block layer to always pass correctly
> typed QDicts to the BlockDriver methods.  Also beyond my reach.

I tried and failed.

(http://lists.nongnu.org/archive/html/qemu-block/2018-05/msg00061.html)

But that will probably be an intermediate step before we get to
BlockdevOptions.

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

I'm calling qdict_stringify_for_keyval() only from a single function,
and it's immediately followed by a qdict_crumple(), so I suppose
replacing those invocations in my series would be trivial.

OTOH, just using qdict_stringify_for_keyval() in your
qdict_crumple_for_keyval_qiv() function would be trivial as well. :-)
But then again, it probably makes more sense to combine the two
functions because I too assumed the QDict to be flattened before the
function call for simplification.  This assumption makes more sense if
the function proceeds to call qdict_crumple().

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

Sounds good!

> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  block/nfs.c           |  2 +-
>  block/parallels.c     |  2 +-
>  block/qcow.c          |  2 +-
>  block/qcow2.c         |  2 +-
>  block/qed.c           |  2 +-
>  block/rbd.c           |  2 +-
>  block/sheepdog.c      |  2 +-
>  block/vhdx.c          |  2 +-
>  block/vpc.c           |  2 +-
>  include/block/qdict.h |  1 +
>  qobject/block-qdict.c | 57 +++++++++++++++++++++++++++++++++++++++++++
>  11 files changed, 67 insertions(+), 9 deletions(-)

[...]

> @@ -513,6 +516,60 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
>      return NULL;
>  }
>  
> +/**
> + * qdict_crumple_for_keyval_qiv:
> + * @src: the flat dictionary (only scalar values) to crumple
> + * @errp: location to store error
> + *
> + * Like qdict_crumple(), but additionally transforms scalar values so
> + * the result can be passed to qobject_input_visitor_new_keyval().
> + *
> + * The block subsystem uses this function to prepare its flat QDict
> + * with possibly confused scalar types for a visit.  It should not be
> + * used for anything else, and it should go away once the block
> + * subsystem has been cleaned up.
> + */
> +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";
> +            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;
> +}
> +
>  /**
>   * qdict_array_entries(): Returns the number of direct array entries if the
>   * sub-QDict of src specified by the prefix in subqdict (or src itself for
> 

Looks functionally very much equivalent to my version, so no complaints
from me.

Max

signature.asc
Description: OpenPGP digital signature