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

[Markus Armbruster]

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?

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.

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.

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.

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

Can we still back out the list part of the convention, and use repeated
keys instead?

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.

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.