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

[Daniel P. Berrange]

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

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