Re: [Qemu-devel] [Qemu-block] [PATCH v14 10/21] qapi: permit auto-creating nested structs

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

> On Thu, Oct 06, 2016 at 05:51:57PM +0200, Kevin Wolf wrote:
>> Am 06.10.2016 um 17:39 hat Daniel P. Berrange geschrieben:
>> > On Thu, Oct 06, 2016 at 05:30:05PM +0200, Kevin Wolf wrote:
>> > > Am 06.10.2016 um 17:18 hat Daniel P. Berrange geschrieben:
>> > > > On Thu, Oct 06, 2016 at 05:10:42PM +0200, Kevin Wolf wrote:
>> > > > > Am 30.09.2016 um 16:45 hat Daniel P. Berrange geschrieben:
>> > > > > > The example is the NetLegacy type which has 3 levels of
>> > > > > > structs. The modern way to represent this in QemuOpts would
>> > > > > > be the dot-separated component approach
>> > > > > > 
>> > > > > >   -net vlan=1,id=foo,name=bar,opts.type=tap,\
>> > > > > >        opts.data.fd=3,opts.data.script=ifup
>> > > > > > 
>> > > > > > The legacy syntax will just be presenting
>> > > > > > 
>> > > > > >   -net vlan=1,id=foo,name=bar,type=tap,fd=3,script=ifup
>> > > > > > 
>> > > > > > So we need to auto-create 3 levels of struct when visiting.
>> > > > > > 
>> > > > > > The implementation here will enable visiting in both the
>> > > > > > modern and legacy syntax, compared to OptsVisitor which
>> > > > > > only allows the legacy syntax.
>> > > > > 
>> > > > > So you're actually introducing the modern syntax only now?
>> > > > 
>> > > > No, the modern syntax is fully implemented by patch 8.
>> > > 
>> > > "now" in the sense of "in this series" (because this means that there is
>> > > no external API to preserve yet).
>> > 
>> > Well the syntax implemented in patch 8 is designed to
>> > 100% mirror the QAPI schema structure nesting. I don't
>> > think we want to change that behaviour.
>> 
>> I think patch 8 is fine as it is.
>> 
>> > If there are certain QAPI schemas structs can still
>> > have freedom to change though, its fine to reconsider
>> > them
>> > 
>> > > 
>> > > > This patch is about adding hacks for the legacy syntax used
>> > > > by the OptsVisitor. The OptsVisitor didn't interpret struct
>> > > > nesting at all, so everything looked flat - this only works
>> > > > as long as you don't have the same key used in multiple
>> > > > structs at different levels, so is not useful as a general
>> > > > approach - it only works by luck really.
>> > > > 
>> > > > > I don't think an "opts.data." prefix makes a lot of sense. I suspect 
>> > > > > the
>> > > > > schema looks this way only because we didn't have flat unions in 1.2.
>> > > > >
>> > > > > So, considering that it is a purely internally used type not visible 
>> > > > > in
>> > > > > QMP, would it make sense to change NetLegacy to be a flat union 
>> > > > > instead,
>> > > > > with NetLegacyOptions as the common base? Then you get the same flat
>> > > > > namespace that we always had and that makes much more sense as an 
>> > > > > API.
>> > > > 
>> > > > Changing that will impact on the QMP data structure, so I don't think
>> > > > we can do that.
>> > > 
>> > > I don't see this type used in QMP at all. It's only used for command
>> > > line parsing, and only with the OptsVisitor, so I think we're fine if we
>> > > flatten it now.
>> > 
>> > Ok, yes, it seems "NetLegacy" is only used in CLI arg parsing, so we
>> > do have some freedom there.
>> > 
>> > This patch was also needed for -numa handling too - again we might have
>> > some flexibility to flatten that.
>> 
>> NumaOptions is also unused in QMP, so it's the same thing.

For better or worse, the QAPI schema doesn't separate externally visible
stuff from purely internal stuff.  Useful trick to see what's external:

    $ python -B scripts/qapi-introspect.py -cu -p scratch- qapi-schema.json

This generates scratch-qmp-introspect.c describing the external QMP
interface with unmasked type names (-u).  Anything not mentioned there
can be changed freely.

I'd like to see patches flattening simple unions that aren't externally
visible.  Flat carries a bit of notational overhead in the schema, but I
hope to address that once the QAPI queue is under control.

>> If these two are the only options that need the behaviour, I would
>> prefer if we changed the QAPI schema to flatten them, and then we could
>> save ourselves some complexity by dropping this patch.
>
> Agreed, the fewer hacks like this that we need the better.

Indeed.  Please find out which hacks we actually need.

I believe the best way to keep the warts in check is getting rid of the
options visitor (done in this series) and the string visitor (hopefully
enabled by this series).