Re: [Qemu-block] [Qemu-devel] [PATCH v14 13/21] qdict: allow qdict_crumple to accept compound types as values

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 13.10.2016 um 14:35 hat Markus Armbruster geschrieben:
>> Cc: Kevin for discussion of QemuOpts dotted key convention
>> 
>> "Daniel P. Berrange" <address@hidden> writes:
>> 
>> > Currently qdict_crumple requires a totally flat QDict as its
>> > input. i.e. all values in the QDict must be scalar types.
>> >
>> > In order to have backwards compatibility with the OptsVisitor,
>> > qemu_opt_to_qdict() has a new mode where it may return a QList
>> > for values in the QDict, if there was a repeated key. We thus
>> > need to allow compound types to appear as values in the input
>> > dict given to qdict_crumple().
>> >
>> > To avoid confusion, we sanity check that the user has not mixed
>> > the old and new syntax at the same time. e.g. these are allowed
>> >
>> >    foo=hello,foo=world,foo=wibble
>> >    foo.0=hello,foo.1=world,foo.2=wibble
>> >
>> > but this is forbidden
>> >
>> >    foo=hello,foo=world,foo.2=wibble
>> 
>> I understand the need for foo.bar=val.  It makes it possible to specify
>> nested dictionaries with QemuOpts.
>> 
>> The case for foo.0=val is less clear.  QemuOpts already supports lists,
>> by repeating keys.  Why do we need a second, wordier way to specify
>> them?
>
> Probably primarily because someone didn't realise this when introducing
> the dotted syntax.

Can't even blame "someone" for that; it's an obscure, underdocumented
feature of an interface that's collapsing under its load of obscure,
underdocumented features.

On the other hand, that's not exactly a state that allows for *more*
obscure features.

>                    Also because flat QDicts can't represent this.

Explain?

> But even if I realised that QemuOpts support this syntax, I think we
> would still have to use the dotted syntax because it's explicit about
> the index and we need that because the list can contains dicts.
>
> Compare this:
>
>     driver=quorum,
>     child.0.driver=file,child.0.filename=disk1.img,
>     child.1.driver=host_device,child.1.filename=/dev/sdb,
>     child.2.driver=nbd,child.2.host=localhost
>
> And this:
>
>     driver=quorum,
>     child.driver=file,child.filename=disk1.img,
>     child.driver=host_device,child.filename=/dev/sdb,
>     child.driver=nbd,child.host=localhost

Aside: both are about equally illegible, to be honest.

> For starters, can we really trust the order in QemuOpts so that the
> right driver and filename are associated with each other?

The order is trustworthy, but...

> We would also have code to associate the third child.driver with the
> first child.host (because file and host_device don't have a host
> property). And this isn't even touching optional arguments yet. Would
> you really want to implement or review this?

... you're right, doing lists by repeating keys breaks down when
combined with the dotted key convention's use of repetition to do
structured values.

Permit me to digress.

QemuOpts wasn't designed for list-values keys.  Doing lists by
repetition was clever.

QemuOpts wasn't designed for structured values.  Doing structured values
by a dotted key convention plus repetition was clever.

And there's the problem: too much cleverness, not enough "this is being
pushed way beyond its design limits, time to replace it".

For me, a replacement should do structured values by providing suitable
*value* syntax instead of hacking it into the keys:

    { "driver": "quorum",
      "child": [ { "driver": "file", "filename": "disk1.img" },
                 { "driver": "host_device", "filename=/dev/sdb" },
                 { "driver": "nbd", "host": "localhost" } ] }

Note how the structure is obvious.  It isn't with dotted keys, not even
if you order them sensibly (which users inevitably won't).

Also not that the value needs to be parsed by QemuOpts!  You can't leave
it to the consumer of the QemuOpts, because if you did, you'd have to
escape the commas.

If you'd rather invent syntax closer to QemuOpts than reuse JSON, you
could try

    driver=quorum,
    child=[{ driver=file, filename=disk1.img },
           { driver=host_device, filename=/dev/sdb },
           { driver=nbd, host=localhost } ]

I'd go with some existing syntax, though.  The one we already use is
JSON.

End of digression.

>> Note that this second way creates entirely new failure modes and
>> restrictions.  Let me show using an example derived from one in
>> qdict_crumple()'s contract:
>> 
>>     foo.0.bar=bla,foo.eek.bar=blubb
>> 
>>     Without the dotted key convention, this is perfectly fine: key
>>     "foo.0.bar" has the single value "bla", and key "foo.eek.bar" has
>>     the single value "blubb".  Equivalent JSON would be
>> 
>>       { "foo.0.bar": "bla", "foo.eek.bar": "blubb" }
>> 
>>     With just the struct convention, it's still fine: it obviously means
>>     the same as JSON
>> 
>>       { "foo": { "0": { "bar": "bla" }, "eek": { "bar": "blubb" } } }
>> 
>>     Adding the list convention makes it invalid.  It also outlaws a
>>     bunch of keys that would be just fine in JSON, namely any that get
>>     recognized as list index.  Raise your hand if you're willing to bet
>>     real money on your predictions of what will be recognized as list
>>     index, without looking at the code.  I'm not.
>
> If you're adding dict keys to the schema that could be reasonably parsed
> as a list index, I think you're doing something wrong. I would agree
> that this is a problem if we were talking about user-supplied values,
> but it's just keys that are defined by qemu.

Your argument would be stronger if we didn't have such a disappointing
track record on sensible naming.  We've failed at picking a consistent
rules for names, let alone enforcing them.  Instead, we have local
rules, enforced to varying degrees, and of course inconsistent with each
other.  We even have code to mangle names, so that they conform to some
other set of rules.

We should not extend an interface in a way that makes it rely on certain
rules for names without writing down and enforcing these rules.

You did it *locally* for block stuff, and got away with it, because you
control the keys (hopefully), and nobody (including probably you and
very much me) wants to think about this particular mess unless it's
absolutely, obviously, embarrassingly necessary.

Which is what he get to do now, because now we're discussing to adopt
your local thing for general use, with keys no single person really
knows, let alone controls.

Your dotted key convention requires two rules: 1. names must not look
like integers, and 2. names must not contain '.'.

We can avoid rule 2 by requiring '.' to be escaped.  Dan's
qdict_crumple() currently does that, to your surprise.  Adding the
escaping to existing options is a compatibility break, however.  So, if
names with '.' already exist, we can either break compatibility by
renaming them, or break it by requiring the '.' to be escaped.

The names in question are the names declared in QemuOptDesc (easy enough
to find) plus any names that the code may use with empty QemuOptDesc
(harder).  Empty QemuOptDesc are used with the following option groups:

* "drive", qemu_drive_opts in blockdev.c
  Also empty_opts in qemu-io.c and qemu_drive_opts in test-replication.c

* "numa", qemu_numa_opts in numa.c

* "device", qemu_device_opts in qdev-monitor.c

  This one pulls in qdev properties.  Properties violating rule 2 exist.

* "object", qemu_object_opts in vl.c
  Also qemu_object_opts in qemu-img.c, qemu-io.c and qemu-nbd.c (WTF?)

  This one pulls in properties of user-creatable objects.

* "source", qemu_soource_opts (sic!) in qemu-img.c

* "reopen", reopen_opts in qemu-io-cmds.c

* "file", file_opts in qemu-io.c and qemu-nbd.c

* "machine", qemu_machine_opts in vl.c

  This one pulls in machine properties.

* "tpmdev", qemu_tpmdev_opts in vl.c

* "netdev", qemu_netdev_opts in net.c

* "net", qemu_net_opts in net.c

* "userdef", userdef_opts in tests-opts-visitor.c

* "opts_list_03", opts_list_03 in test-qemu-opts.c

* "acpi", qemu_acpi_opts in hw/acpi/core.c

* "smbios", qemu_smbios_opts in smbios.c

Empty QemuOptDesc is used this widely because QemuOpts is too primitive
to support the needs of its users.  Unlike a QAPI schema.  Just sayin'.

Finding the names and assessing how they're impacted by the dotted key
convention is going to be a substantial chunk of work.

>> I'm afraid I have growing doubts regarding the QemuOpts dotted key
>> convention in general.
>> 
>> The convention makes '.' a special character in keys, but only
>> sometimes.  If the key gets consumed by something that uses dotted key
>> convention, '.' is special, and to get a non-special '.', you need to
>> escape it by doubling.  Else, it's not.
>
> Do we really implement escaping by doubling dots? This would be news to
> me.

Dan's PATCH 02/21 does.

> Do we even have a reason to escape dots, i.e. are there any options in
> the schema whose keys contain a dot? I think we took care to avoid such
> things.

I'm afraid we did not.

>> Since the same key can be used differently by different code, the same
>> '.' could in theory be both special and non-special.  In practice, this
>> would be madness.
>> 
>> Adopting the dotted key convention for an existing QemuOpts option, say
>> -object [PATCH 15], *breaks* existing command line usage of keys
>> containing '.', because you now have to escape the '.'.  Dan, I'm afraid
>> you need to show that no such keys exist, or if they exist they don't
>> matter.
>> 
>> I know we have keys containing '.' elsewhere, e.g. device "macio-ide"
>> property "ide.0".  Our chronic inability to consistently restrict names
>> in ABI to something sane is beyond foolish.
>
> I wanted to have a look at this example, but I can only find the string
> "ide.0" used as a bus name in the sources, that is, a value rather than
> a key.
>
> Do you have a pointer to the property definition that you mean?

We've gotten better at hiding property definitions...

"qemu-system-ppc -device macio-ide,help" shows the property:

    macio-ide.ide.0=child<IDE>

The device is not marked no-user, but the obvious attempt to use it
fails:

    $ qemu-system-ppc -device macio-ide
    qemu-system-ppc: Option '-device macio-ide' cannot be handled by this 
machine

This is the Alex's dynamic system bus thingy, which put most of the
system bus devices in help even when they cannot be used.

Perhaps you can make a case why this property cannot be used via
QemuOpts.  If so, great!  There are about three dozen more for you to
check, followed by fourteen more option groups with empty QemuOptDesc,
and a bunch of non-empty QemuOptDesc ;-P

>> It's probably too late to back out the dotted key convention completely.
>> Kevin?
>
> Yes, far too late.

If we're stuck with the existing use of dotted key convention, we're
stuck with it.  But that's no excuse for adding more uses.

If we decide we want to use dotted key convention more widely, we must
document and enforce the naming conventions it requires at least in all
the places where we use it.  As discussed above, this might involve
compatibility breaks, hopefully minor.

> Also, I haven't yet seen even just an idea for an alternative solution.

See digression above.

>> Can we still back out the list part of the convention, and use repeated
>> keys instead?
>
> See above, repeated keys are messy and can't provide the same thing.
>
> Also it's been in use since quorum was introduced in commit c88a1de51,
> February 2014. Pretty sure that this means no anyway.
>
>> If we're stuck with some form of the dotted key convention, can we at
>> least make it a more integral part of QemuOpts rather than something
>> bolted on as an afterthought?  Here's my thinking on how that might be
>> done:
>> 
>> * Have a QemuOptsList flag @flat.
>> 
>> * If @flat, QemuOpts behaves as it always has: the special characters
>>   are ',' and '=', and parsing a key=value,... string produces a list
>>   where each element represents one key=value from the string, in the
>>   same order.
>> 
>> * If not @flat, '.' becomes an additional special character, and parsing
>>   a key=value,... string produces a dictionary, similar to the one you
>>   get now by converting with qemu_opts_to_qdict() and filtering through
>>   qdict_crumple().
>> 
>> The difference to now is that you either always crumple, or not at all,
>> and the meaning of '.' is unambiguous.
>
> Integrating qdict_crumple() functionality in QemuOpts sounds like a
> reasonable next step after Dan's series.
>
>> I wish we had refrained from saddling QemuOpts with even more magic.
>> Compared to this swamp, use of JSON on the command line looks rather
>> appealing to me.
>
> I'd bet most users would disagree with you because they use only simple
> cases.

Use of JSON on the command line when and where you need nesting, not
everywhere.  The simple cases can stay exactly as they are.

> In more complicated setups, dotted syntax still works (unlike repeated
> options), but becomes a bit verbose because of the repetition of long
> prefixes. If we wanted to attack this, we would have to introduce two
> more special characters (brackets) for a more concise nesting syntax.
>
> Kevin