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

2016-10-20 Thread Daniel P. Berrange
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"  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 

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

2016-10-19 Thread Daniel P. Berrange
On Wed, Oct 19, 2016 at 11:25:27AM +0200, Kevin Wolf wrote:
> Am 18.10.2016 um 17:35 hat Markus Armbruster geschrieben:
> > Kevin Wolf  writes:
> > > Of course, you could argue that flat QDicts are the wrong data structure
> > > in the first place and instead of flatting everything we should have
> > > done the equivalent of qdict_crumple from the beginning, but they were
> > > the natural extension of what was already there and happened to work
> > > good enough, so this is what we're currently using.
> > 
> > We didn't "flatten everything", because QemuOpts is and has always been
> > flat to begin with.  Rather we've started to crumple a few things, and
> > in a separate layer.
> 
> That's the QemuOpts point of view.
> 
> I was talking from a block layer view. There we get data in two
> different formats, QMP and the command line. The first is structured,
> the second is flat. We need to make both uniform before we can pass them
> to the actual block layer functions.
> 
> The current code chose to flatten what we get from QMP blockdev-add and
> use that throughout the block layer. We could instead have decided that
> we leave the blockdev-add input as it is, crumple what we get from
> QemuOpts and use nested QObjects throughout the block layer.

I would very much like to see BlockdevOptions structs passed around
the block drivers, instead of QemuOpts - I think it'd lead to much
clearer code than the QemuOpts stuff we have today IMHO

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



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

2016-10-19 Thread Kevin Wolf
Am 18.10.2016 um 17:35 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> > Of course, you could argue that flat QDicts are the wrong data structure
> > in the first place and instead of flatting everything we should have
> > done the equivalent of qdict_crumple from the beginning, but they were
> > the natural extension of what was already there and happened to work
> > good enough, so this is what we're currently using.
> 
> We didn't "flatten everything", because QemuOpts is and has always been
> flat to begin with.  Rather we've started to crumple a few things, and
> in a separate layer.

That's the QemuOpts point of view.

I was talking from a block layer view. There we get data in two
different formats, QMP and the command line. The first is structured,
the second is flat. We need to make both uniform before we can pass them
to the actual block layer functions.

The current code chose to flatten what we get from QMP blockdev-add and
use that throughout the block layer. We could instead have decided that
we leave the blockdev-add input as it is, crumple what we get from
QemuOpts and use nested QObjects throughout the block layer.

> I now think this is the wrong approach.  We have clearly outgrown flat
> options.  We should redo QemuOpts to support what we need instead of
> bolting on an optional crumpling layer.
> 
> I guess I reached the place you described, just on a different path :)

Yes, I think the conclusion is easy to agree on.

> > Okay, so I like JSON. It's a great format for our monitor protocol. We
> > even have pretty printers so that it's more or less readable as output
> > for human users. However, there is one thing for which it is horrible:
> > Getting user input.
> >
> > Yes, getting rid of the comma escaping is a first step, but typing JSON
> > on the command line remains a PITA. Mostly because of the quotes (which
> > you probably have to escape), but also things like the useless outer
> > brackets.
> 
> As long as you don't need "'" in your JSON, you can simply enclose in
> "'" and be done.  Since "'" can only occur in JSON strings, and the same
> strings would be present in any other syntax, any other syntax would
> be pretty much the same PITA.

I've written enough scripts (qemu-iotests cases) that produce JSON with
shell variables in it, so if anything you can use "" quoting for the
shell and make use of qemu's extension that '' is accepted in JSON, too.

Anyway, the quotes aren't only nasty because of the escaping, but also
just because I have to type them.

> >> 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 } ]
> >
> > This looks a lot more agreeable as something that I have to type on the
> > command line.
> >
> > What's more, this is a direct extension of the existing syntax. You
> > complained about how the step from simple configurations with -hda to
> > more complex ones with -drive completely changes the syntax (and rightly
> > so). Going from simple QemuOpts syntax to doing JSON as soon as you get
> > structured values and lists would be another similar step. In contrast,
> > the new syntax you're suggesting above is a natural extension of what's
> > there.
> 
> Point taken.  I just don't like inventing syntax, because as a rule, way
> too much syntax gets invented, and almost always badly.
> 
> >> I'd go with some existing syntax, though.  The one we already use is
> >> JSON.
> >
> > The one we already use in this context is comma separated key/value
> > pairs as parsed by QemuOpts.
> 
> Which is flat, and flat doesn't cut it anymore.
> 
> If you make it non-flat with dotted key convention, the dotted key
> convention becomes part of the syntax.  Counts as inventing syntax in my
> book.

Yes, it is. Though in the context of command line options, dotted syntax
is an invention already made, whereas JSON would be a new invention.
(Well, not completely, because for block devices we already have json: -
but that works a little different again.)

> >> 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.
> >
> > I don't think we should support any escaping and rather forbid '.'
> > completely in names.
> 
> I think we should adopt QAPI's naming rules.

Which includes what I said, so fine with me.

> > If you're concerned about compatibility issues if we find a dot in a
> > name only in one of the later 

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

2016-10-18 Thread Markus Armbruster
Kevin Wolf  writes:

> Am 17.10.2016 um 16:50 hat Markus Armbruster geschrieben:
>> Kevin Wolf  writes:
>> 
>> > Am 13.10.2016 um 14:35 hat Markus Armbruster geschrieben:
>> >> Cc: Kevin for discussion of QemuOpts dotted key convention
>> >> 
>> >> "Daniel P. Berrange"  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.
>
> I don't really think we're introducing more obscure features here, but
> rather trying to implement a single, and rather straightforward, way as
> the standard.
>
> Dotted syntax for hierarchy has actually plenty of precedence in qemu if
> you look a bit closer (the block layer, -global, -device foo,help, even
> the bus names you mentioned below are really just flattened lists), so
> we're only making things more consistent.
>
>> >Also because flat QDicts can't represent this.
>> 
>> Explain?
>
> Repeated options are parsed into QLists. If you keep it at that without
> flattening you have at least a QDict that contains a QList that contains
> simple types. This is not flat any more.

*All* options are parsed into a (non-QList) list.  That's what is flat.

Only when you start crumpling things go beyond flat, and QDict/QList
come into play.

> Of course, you could argue that flat QDicts are the wrong data structure
> in the first place and instead of flatting everything we should have
> done the equivalent of qdict_crumple from the beginning, but they were
> the natural extension of what was already there and happened to work
> good enough, so this is what we're currently using.

We didn't "flatten everything", because QemuOpts is and has always been
flat to begin with.  Rather we've started to crumple a few things, and
in a separate layer.

I now think this is the wrong approach.  We have clearly outgrown flat
options.  We should redo QemuOpts to support what we need instead of
bolting on an optional crumpling layer.

I guess I reached the place you described, just on a different path :)

>> > 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.
>
> Not sure about equally, but let's agree on "both are illegible".
>
>> > 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.
>> 
>> 

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

2016-10-18 Thread Markus Armbruster
Eric Blake  writes:

> On 10/17/2016 09:50 AM, Markus Armbruster wrote:
>>> 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.
>
>> 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" } ] }
>
> Possible hack solution:
>
> QemuOpts already special-cases id=.  What if we ALSO make it
> special-case a leading json=?  Shown here with shell quoting, the above
> example of creating a Quorum -drive argument could then be:
>
> -drive json='
> { "driver": "quorum",
>   "child": [ { "driver": "file", "filename": "disk1.img" },
>  { "driver": "host_device", "filename=/dev/sdb" },
>  { "driver": "nbd", "host": "localhost" } ] }
> '
>
> As far as I know, we don't have 'json' as any existing QemuOpts key (do
> we? A full audit may be better than my quick git grep '"json"').  Thus,
> if QemuOpts sees a leading json=, it hands off the rest of the string to
> the same parser as we use for QMP, where we no longer have to escape
> commas (even nicer than the drive hack where we support
> filename=json:{...} but have to double up all commas to make it through
> the QemuOpts layer).  Encountering json= as anything other than the
> first option would be an error, and you would be unable to combine a
> json= option with any other old-style option.  In other words, the use
> of leading json= would be the switch for whether to do old-style parsing
> or to use a saner syntax for everything else that needs structure, on a
> per-argument basis.

Slight variation: omit the 'json=' and recognize the '{':

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

As always when extending haphazardly defined syntax, the question to ask
is whether this makes the syntax (more) ambiguous.

So what is the option argument syntax now?  The abstract syntax is
simple enough: "list of (key, value) pairs".  For the concrete syntax,
we need to study opts_do_parse().

Each iteration of its loop accepts an abstract (key, value).  It first
looks for the leftmost '=' and ','.  Cases:

1. There is no '=', or it is to the right of the leftmost ','.  In other
words, this (key, value) can only be key with an implied value or a
value with an implied key.

1a. If this is the first iteration, and we accept an implied key, this
is a value for the implied key.  We consume everything up to the first
non-escaped ','.  This may be more than the leftmost ',' we found above.
The consumed string with escapes processed is the value.

1b. Else, this is a key with an implied value.  We consume everything up
to the leftmost ',' (no escaping here).

1b1. If the consumed string starts with "no", the key is everything after
the "no" and the value is "off".

1b2. Else the key is the string and the value is "on".

2. This is a key and a value.  We first consume everything up to the
leftmost '=' (no escaping here).  The consumed string is the key.  We
then consume '='.  Finally, we consume everything up to the first
non-escaped ','The consumed string with escapes processed is the value.

Thus, the option argument starts either with a key (case 1b1, 2), "no"
(case 1b2) or a value (case 1a).

Adding JSON object syntax (which always starts with '{') is ambiguous
when a key can start with '{' (case 1b1, 2) or when a value can (case
1a).

Keys starting with '{' are basically foolish.  Let's outlaw them by
adopting QAPI's name rules.

Values starting with '{' are possible.  The implied keys I can remember
don't 

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

2016-10-18 Thread Kevin Wolf
Am 17.10.2016 um 16:50 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > Am 13.10.2016 um 14:35 hat Markus Armbruster geschrieben:
> >> Cc: Kevin for discussion of QemuOpts dotted key convention
> >> 
> >> "Daniel P. Berrange"  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.

I don't really think we're introducing more obscure features here, but
rather trying to implement a single, and rather straightforward, way as
the standard.

Dotted syntax for hierarchy has actually plenty of precedence in qemu if
you look a bit closer (the block layer, -global, -device foo,help, even
the bus names you mentioned below are really just flattened lists), so
we're only making things more consistent.

> >Also because flat QDicts can't represent this.
> 
> Explain?

Repeated options are parsed into QLists. If you keep it at that without
flattening you have at least a QDict that contains a QList that contains
simple types. This is not flat any more.

Of course, you could argue that flat QDicts are the wrong data structure
in the first place and instead of flatting everything we should have
done the equivalent of qdict_crumple from the beginning, but they were
the natural extension of what was already there and happened to work
good enough, so this is what we're currently using.

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

Not sure about equally, but let's agree on "both are illegible".

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

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

2016-10-17 Thread Markus Armbruster
Paolo Bonzini  writes:

>> 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.
>
> In fact there is already "filename=json:{...}" support in the block layer.

Yes.

> By the way, abuse of QemuOpts dates back to 
> http://wiki.qemu.org/Features/QCFG.

That page is from 2011, when QAPI was being implemented.  The idea to
bring the power of the QAPI schema to bear on the command line is sound,
but QCFG never made it past the ideas stage, I'm afraid.  It uses either
dotted keys or braces for nested structs.  It doesn't cover lists.

>> 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.
>
>> * "device", qemu_device_opts in qdev-monitor.c
>> 
>>   This one pulls in qdev properties.  Properties violating rule 2 exist.
>
> Which are they?  Only bus names?

Finding properties is difficult, because we (foolishly!) define them in
code rather than data.  My testing with "info qdm" for all targets
finds:

= xlnx.axi-dma =
xlnx.axi-dma[0]
= xlnx.xps-ethernetlite =
xlnx.xps-ethernetlite[0]
= xlnx.xps-intc =
xlnx.xps-intc[0]
= xlnx.xps-uartlite =
xlnx.xps-uartlite[0]
= xlnx.axi-dma =
xlnx.axi-dma[0]
= xlnx.xps-ethernetlite =
xlnx.xps-ethernetlite[0]
= xlnx.xps-intc =
xlnx.xps-intc[0]
= xlnx.xps-uartlite =
xlnx.xps-uartlite[0]
= cuda =
adb.0
= raven-pcihost =
pci.0
= macio-ide =
ide.0
= mpc8544-guts =
mpc8544.guts[0]
= xlnx.xps-ethernetlite =
xlnx.xps-ethernetlite[0]
= xlnx.xps-intc =
xlnx.xps-intc[0]
= xlnx.xps-uartlite =
xlnx.xps-uartlite[0]
= cuda =
adb.0
= raven-pcihost =
pci.0
= macio-ide =
ide.0
= mpc8544-guts =
mpc8544.guts[0]
= xlnx.xps-ethernetlite =
xlnx.xps-ethernetlite[0]
= xlnx.xps-intc =
xlnx.xps-intc[0]
= xlnx.xps-uartlite =
xlnx.xps-uartlite[0]
= s390-sclp-event-facility =
s390-sclp-events-bus.0
= cgthree =
cg3.reg[0]
cg3.prom[0]
= SUNW,tcx =
tcx.thc[0]
tcx.rblit[0]
tcx.dac[0]
tcx.alt[0]
tcx.tec[0]
tcx.rstip[0]
tcx.blit[0]
tcx.dhc[0]
tcx.prom[0]
tcx.stip[0]

>> * "object", qemu_object_opts in vl.c
>> 
>>   This one pulls in properties of user-creatable objects.
>> 
>> * "machine", qemu_machine_opts in vl.c
>> 
>>   This one pulls in machine properties.
>
>> > > 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
>
> It is a bug that this property is shown in the help, because it's not
> assignable (same for all other child<> properties).

Let's fix it then.

>  I'd rather declare
> other occurrences of "." in user-accessible property names to be bugs,
> and break the ABI if there are any.

I propose to adopt QAPI's rules on permissible names: "All names must
begin with a letter, and contain only ASCII letters, digits, dash, and
underscore" (docs/qapi-code-gen.txt).  QAPI's naming rules get adopted
anyway on QAPIfication, so why wait? :)

Note that this may affect names generated by 

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

2016-10-17 Thread Eric Blake
On 10/17/2016 09:50 AM, Markus Armbruster wrote:
>> 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.

> 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" } ] }

Possible hack solution:

QemuOpts already special-cases id=.  What if we ALSO make it
special-case a leading json=?  Shown here with shell quoting, the above
example of creating a Quorum -drive argument could then be:

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

As far as I know, we don't have 'json' as any existing QemuOpts key (do
we? A full audit may be better than my quick git grep '"json"').  Thus,
if QemuOpts sees a leading json=, it hands off the rest of the string to
the same parser as we use for QMP, where we no longer have to escape
commas (even nicer than the drive hack where we support
filename=json:{...} but have to double up all commas to make it through
the QemuOpts layer).  Encountering json= as anything other than the
first option would be an error, and you would be unable to combine a
json= option with any other old-style option.  In other words, the use
of leading json= would be the switch for whether to do old-style parsing
or to use a saner syntax for everything else that needs structure, on a
per-argument basis.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature


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

2016-10-17 Thread Paolo Bonzini
> 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.

In fact there is already "filename=json:{...}" support in the block layer.
By the way, abuse of QemuOpts dates back to http://wiki.qemu.org/Features/QCFG.

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

> * "device", qemu_device_opts in qdev-monitor.c
> 
>   This one pulls in qdev properties.  Properties violating rule 2 exist.

Which are they?  Only bus names?

> * "object", qemu_object_opts in vl.c
> 
>   This one pulls in properties of user-creatable objects.
> 
> * "machine", qemu_machine_opts in vl.c
> 
>   This one pulls in machine properties.

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

It is a bug that this property is shown in the help, because it's not
assignable (same for all other child<> properties).  I'd rather declare
other occurrences of "." in user-accessible property names to be bugs,
and break the ABI if there are any.

Paolo



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

2016-10-17 Thread Markus Armbruster
Kevin Wolf  writes:

> Am 13.10.2016 um 14:35 hat Markus Armbruster geschrieben:
>> Cc: Kevin for discussion of QemuOpts dotted key convention
>> 
>> "Daniel P. Berrange"  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
>>   

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

2016-10-13 Thread 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"  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 

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

2016-10-13 Thread Markus Armbruster
Cc: Kevin for discussion of QemuOpts dotted key convention

"Daniel P. Berrange"  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.