Re: [Qemu-devel] [PATCH v14 12/21] option: allow qemu_opts_to_qdict to merge repeated options

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

On Fri, Oct 21, 2016 at 01:09:07PM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
> > On Wed, Oct 12, 2016 at 07:46:00PM +0200, Markus Armbruster wrote:
> >> "Daniel P. Berrange" <berrange@redhat.com> writes:
> >> 
> >> > If given an option string such as
> >> >
> >> >   size=1024,nodes=10,nodes=4-5,nodes=1-2,policy=bind
> >> >
> >> > the qemu_opts_to_qdict() method will currently overwrite
> >> > the values for repeated option keys, so only the last
> >> > value is in the returned dict:
> >> >
> >> >     size=QString("1024")
> >> >     nodes=QString("1-2")
> >> >     policy=QString("bind")
> >> >
> >> > With this change the caller can optionally ask for all
> >> > the repeated values to be stored in a QList. In the
> >> > above example that would result in 'nodes' being a
> >> > QList, so the returned dict would contain
> >> >
> >> >     size=QString("1024")
> >> >     nodes=QList([QString("10"),
> >> >                  QString("4-5"),
> >> >                  QString("1-2")])
> >> >     policy=QString("bind")
> >> >
> >> > Note that the conversion has no way of knowing whether
> >> > any given key is expected to be a list upfront - it can
> >> > only figure that out when seeing the first duplicated
> >> > key. Thus the caller has to be prepared to deal with the
> >> > fact that if a key 'foo' is a list, then the returned
> >> > qdict may contain either a QString or a QList for the
> >> > key 'foo'.
> >> >
> >> > In a third mode, it is possible to ask for repeated
> >> > options to be reported as an error, rather than silently
> >> > dropping all but the last one.
> >> 
> >> To serve as a replacement for the options visitor, this needs to be able
> >> to behave exactly the same together with a suitably hacked up QObject
> >> input visitor.  Before I dive into the actual patch, let me summarize
> >> QemuOpts and options visitor behavior.
> [...]
> >> Unlike the options visitor, this patch (judging from your description)
> >> makes a list only when keys are repeated.  The QObject visitor will have
> >> to cope with finding both scalars and lists.  When it finds a scalar,
> >> but needs a list, it'll have to wrap it in a list (PATCH 09, I think).
> >> When it finds a list, but needs a scalar, it'll have to fish it out of
> >> the list (where is that?).
> >
> > If my code finds a list but wants a scalar, it is reporting an
> > error.
> 
> I'm afraid that breaks things like
> 
>     --numa node,cpus=0,cpus=2,mem=512,nodeid=0,nodeid=0
> 
> The options visitor inteprets cpus=0,cpus=2 as a list [0,2] (because
> NumaNodeOptions member @cpus has a list type) and nodeid=0,nodid=0 as
> integer 0 (because member @nodeid has a scalar type).

Right, but that command line syntax is arguably broken already - the
app/user invoking QEMU is buggy, so I felt it was justifiable to
change from silently ignoring the app error to telling them of their
mistake.

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