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

["Daniel P. Berrange" <berrange@redhat.com>]

On Thu, Oct 13, 2016 at 02:35:38PM +0200, Markus Armbruster wrote:
> Cc: Kevin for discussion of QemuOpts dotted key convention
> 
> "Daniel P. Berrange" <berrange@redhat.com> 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?

Two reasons I did this. First blockdev already uses this foo.0=val
syntax, and I wanted to be compatible with blockdev, so it could be
converted to use this new code.

Second, using foo.0 syntax means that you can unambigously determine
whether a key is going to be a scalar or a list. This lets the
qdict_crumple() method convert the QemuOpts to a QDict without
needing to know anything about the QAPI schema.

Of course I later had to add hacks to the visitor to cope with
the bare repeated key syntax, so I lost some of that benefit.

Personally I still prefer the unambiguous syntax as it lets us
give clear error messages when users do unexpected things, instead
of say, silently ignoring all but the last key.

> 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 checked the things that I converted (eg -net, -object, -numa, etc),
but I didn't check -device since that's processed using different code.

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

The only issue with dropping the dotted list convention is compat
with the block layer code - we couldn't easily use this new visitor
logic to turn -drive into a QAPI BlockOptions object.  Kevin's new
-blockdev arg would potentially be ok with it since its a new arg,
but IIUC, we would have to do some cleanup inside various block
driver impls, since block layer doesn't use the QAPI objects
internally - they all get converted back into QemuOpts :-(

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

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|