Re: [Qemu-block] [Qemu-devel] [RFC 0/7] block: Try to use correctly typed blockdev options

[Max Reitz]

On 2018-05-03 10:16, Markus Armbruster wrote:
> Max Reitz <address@hidden> writes:
> 
>> (Sorry, Markus, sorry, Kevin, if this series makes you angry.)
> 
> Anger?  Nah.  Gallows humor :)

Good!  I like to make people happy wherever I can.

>> The subject says it all, I think.  The original issue I was assigned to
>> solve is this:
>>
>>     $ ./qemu-img info --image-opts driver=null-co,size=42
>>     image: json:{"driver": "null-co", "size": "42"}
>>     [...]
>>
>> As you can see, "size" gets a string value in the json:{} object
>> although it is an integer, according to the QAPI schema.  (Buglink:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1534396)
>>
>> My first approach to fix this was to try to pipe the full_open_options
>> dict generated in bdrv_refresh_filename() through a keyval input visitor
>> and then an output visitor (which would have depended on my filename
>> series).  This did not work out because bs->options (where all of the
>> reconstructed options come from) may either be correctly typed or not,
>> and keyval chokes on non-string values.  I could have probably converted
>> bs->options to fully string values before trying to pipe it through
>> keyval, but I decided I didn't like that very much.  (I suppose I can be
>> convinced otherwise, though.)
> 
> See also thread
> 
>     [RFC][BROKEN] rbd: Allow configuration of authentication scheme"
>     Message-Id: <address@hidden>
>     https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg00618.html
> 
> in particular the qdict_stringify_entries() proposal and its discussion.

Ah, right.

So if I did that I should note that the limitation below "works for the
more common cases" would still apply, of course.  The main difference is
that it wouldn't be so important anymore because at least it wouldn't
affect other parts of the block layer's option parsing, but only
generation of json:{} filenames.

>> So I decided to venture into the territory of what we actually want at
>> some point: Correctly typed bs->options.  Or, rather, in the end we want
>> bs->options to be BlockdevOptions, but let's not be too hasty here.
>>
>> So it turns out that with really just a bit of work we can separate the
>> interfaces that generate correctly typed options (e.g. blockdev-add)
>> from the ones that generate pure string-valued options (e.g. qemu-img
>> --image-opts) rather well.  Once we have done that, we can pipe all of
>> the pure string options through keyval and back to get correctly typed
>> options.
>>
>> So far the theory.  Now, in practice we of course have pitfalls, and
>> those are not addressed by this series, which is the main reason this is
>> an RFC.
>>
>> The first pitfall (I can see...) is that json:{} allows you to specify
>> mixed options -- some values are incorrectly strings, others are
>> non-strings.  keyval cannot cope with that, so the result after this
>> series is that those options end up in bs->options just like that.  I
>> suppose we could just forbid that mixed-typed case, though, and call it
>> a bug fix.
> 
> Related: QMP device_add and netdev_add convert their arguments from
> QDict to QemuOpts with qemu_opts_from_qdict(), and therefore accept
> string in addition to number / bool.  If I remember correctly, Eric
> Blake's patches to QAPIfy netdev_add got stuck because they didn't
> replicate that behavior.
> 
> I think we should reject inappropriate strings.  If we think nobody's
> relying on it, we can call it a bug and just fix it.  If we don't dare,
> we need to go through the deprecation ritual.  We should've done that
> long ago, but better now than never.

Sure we should reject them, but we can only do that once either
everything well-formed is converted automatically correctly, or once we
have deprecated the rest (which implies having a QAPI alternative).

> Same for json:{}.

Right.

>> The second problem (and I think the big reason why we have not
>> approached this issue so far) is that there are options which you can
>> give as strings, but which are not covered by the schema.  In such a
>> case, the input visitor will reject the QDict and we will just use it
>> as-is, that is, full of strings.  Now that is not what we want in the
>> end, of course -- we want everything to be converted into something that
>> is covered by the schema.
> 
> Can you give an example?

I looked into qcow2's overlap checking the other day but I was amazed
that someone actually managed to convert my mess into a proper QAPI
definition.

Well, rbd's password-secret comes to mind.  I think we should be good
apart from that...?

Oh no, I just stumbled over bdrv_parse_filename(). m(
I totally missed that.  This has the following effect:

$ ./qemu-img info --image-opts \
    driver=blkdebug,image.driver=null-co,align=512
image: json:{"image": {"driver": "null-co"}, "driver": "blkdebug",
"align": 512}

$ ./qemu-img info --image-opts \
    driver=blkdebug,x-image=null-co://,align=512
image: json:{"image": {"driver": "null-co"}, "driver": "blkdebug",
"align": "512"}

x-image (which is usually set by blkdebug_parse_filename()) is taken by
blkdebug_open() to open the "image" child.  But before that it breaks
conversion of the whole thing into BlockdevOptions, of course...

Of course that is cheating.  You're not allowed to use x-image like
that, actually, qemu just doesn't complain about it (yet).

So if you use it the "proper" way:

$ x86_64-softmmu/qemu-system-x86_64 -nodefaults \
    -drive if=none,driver=raw,file=blkdebug::null-co://,file.align=512 \
    -monitor stdio
QEMU 2.12.50 monitor - type 'help' for more information
(qemu) info block
none0 (#block238): json:{"driver": "raw", "file": {"image": {"driver":
"null-co"}, "driver": "blkdebug", "align": "512"}} (raw)

Well... Looks like the same issue, but it's different, actually.
blkdebug_parse_filename() only adds "x-image" after the conversion to
BlockdevOptions and back, so that can't be the thing that breaks the
conversion here.  No, the thing here is the "filename" option which is
not something that blkdebug supports according to the QAPI schema...

(And there are many protocol drivers like this, where the filename can
be very complex and is decomposed into proper QAPI options by
bdrv_parse_filename().  Those filenames of course are not part of the
QAPI schema, then.)

Of course the fun doesn't end here.  rbd for instance has a filename
that is decomposed into things that cannot be represented with QAPI at
all[1].  It has these great arbitrary "keyvalue-pairs" that just bypass
everything and go right into rados_conf_set().

The rest is rather tame, they just produce correct QAPI options
(although sometimes stringified) from their filename.


[1] Well, technically the same is true for blkdebug and blkverify.  The
options their filenames are decomposed into are again complex filenames
which need to go through bdrv_parse_filename() themselves.  But the
difference is that after they have opened their children, they can
discard those non-QAPI options because opening those child nodes
automatically produces the proper QAPI equivalent.

So all they need to do is keep the child filenames around somewhere
until their bdrv_open() implementation, and then they can discard it
because we now know the proper QAPI options for their children.

>> My reasoning for sending this series anyway is that it doesn't make
>> things worse (bs->options is a mess already, you can never be certain
>> that it contains correctly typed values or just strings or both), and
>> that it at least gives a starting point from which we can continue on.
>> After this series, we have a clear separation between the interfaces
>> that use purely string values and the ones that provide correct typing
>> (well, and json:{}).
>>
>> Oh, and it fixes the above BZ for the more common cases.
> 
> "for the more common cases", ha!

Yeah...  Well, if it had been only for the two cases I mentioned in the
cover letter (mixed-type json:{} and string options we don't have a QAPI
equivalent for yet), it would have been pretty much fine.  But now with
the filename chaos...  That's a bit different.


OK, collection time.

Option 1: Implement qdict_stringify() and do the whole magic in
bdrv_refresh_filename(), only for expressing json:{} filenames.  One
issue with this is that it would completely choke on rbd's
keyvalue-pairs.  (But really, who cares.)
OK, I've just seen that we could handle null just well...  Although
floats seem like a funny thing to do (albeit not impossible).

Option 2: Do the whole bdrv_parse_filename() thing before typing the
QDict.  Will choke on rbd's keyvalue-pairs as well, so not better in any
way.

Option 3: Keep doing what this series did.  Find out why "filename" is
an option in the QDict in the first place, because I would have assumed
that you need to specify it as the @filename parameter of bdrv_open()
instead of through the QDict.  Secondly, bdrv_parse_filename()
implementations would need to always put correctly typed values into the
QDict (easy fix).
The advantage of this is that it could cope with the keyvalue-pairs
option because it doesn't exist when the typing is done; and it would
just end up as a string in the json:{} object (which qemu could
interpret).  The question is whether this effort would be worth it.

I suppose I'll look into #3 for a bit if only out of curiosity, and if
that doesn't work, fall back to #1.

Max

signature.asc
Description: OpenPGP digital signature