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

[Kevin Wolf]

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. Also because flat QDicts can't represent this.

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

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

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?

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

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

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.

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

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

Yes, far too late.

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

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

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