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

[Kevin Wolf <kwolf@redhat.com>]

Am 18.10.2016 um 17:35 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> 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 users: We can handle it. If you then
> > stumble across an obscure "weird.name" property where the dot is really
> > part of the name and not already representing structure, you can still
> > artificially reinterpret it as a nested structure with a single field
> > even if logically that's not what it is... Might be a bit ugly, but
> > keeps it working.
> 
> Yes, that's how we'd do backward compatibility, if needed.
> 
> So, what can we do?  Perhaps something like this:
> 
> * Pick a concrete syntax that supports nested stuff.  Proposals on the
>   table so far are
> 
>   1. Dotted key convention as used in the block layer
> 
>   2. Array syntax as used in QOM's automatic arrayification (not a
>      complete solution, mentioned because it conflicts with the
>      previous)
> 
>   3. JSON
> 
>   4. JSON with ':' replaced by '=', double-quotes and the outermost pair
>      of braces dropped.

Biased block layer view on what we ideally want to have in the end:
Anthony's QCFG proposal plus a [] syntax for lists.

This is more or less what you describe in option 4, except that the
existing dotted syntax is still supported. It is part of the ABI by now,
so we can't get rid of it anyway, and supporting it everywhere is not
only more consistent but probably also easier than making it a special
case.

The tricky part here is the {} and [] syntax for dicts and lists:
Parsing this requires a schema if you don't want to restrict string
values to not contain any of these characters. Such a restriction would
be incompatible, so we probably can't make it.

Essentially this means that we're back to "QAPIfy everything" before we
can make this work. Leaves us with implementing only the dotted syntax
subset for now, which is equally powerful, even if a lot more verbose.

> * Replace the QemuOpts internal representation: QDict instead of list.
> 
> * Replace the QemuOpts parser.
> 
> * Add suitable struct- and list-valued accessors.
> 
>   qdict_crumple(qemu_opt_to_qdict(...), true, &errp) gets replaced by a
>   trivial "get the whole dictionary" operation.
> 
>   Other uses of qemu_opt_to_qdict() need to be rewritten.
> 
> * "Repetition builds a list" backward compatibility.  It's used in only
>   a few places.
> 
> * Possibly "keys with funny characters" backward compatibility.
> 
> Feels doable to me.  2.8 would be ambitious, though: soft freeze in in
> less than two weeks.  However, getting this series into 2.8 has started
> to feel ambitious to me, too.

Let's merge qdict_crumple() at least, this would allow us to add the SSH
and NBD blockdev-add support for 2.8, possibly NFS to. No command line
syntax involved at all there. I can take the patch through my tree if
nobody objects.

With -blockdev I'll have to wait for the final decision, but if we
decide to use a syntax that includes dotted syntax (possibly as a
subset), I think this series would provide the right external interface
even if there is still some infrastructure cleanup work to do
internally.

Kevin