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

[Daniel P. Berrange]

On Wed, Sep 28, 2016 at 10:35:05AM +0100, Daniel P. Berrange wrote:
> On Tue, Sep 27, 2016 at 05:03:17PM -0500, Eric Blake wrote:
> > On 09/27/2016 08:13 AM, Daniel P. Berrange wrote:
> > > 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=1024
> > >     nodes=1-2
> > >     policy=bind
> > > 
> > > This adds the ability for the caller to ask that the
> > > repeated keys be turned into list indexes:
> > > 
> > >     size=1024
> > >     nodes.0=10
> > >     nodes.1=4-5
> > >     nodes.2=1-2
> > >     policy=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 'foo' (if only a single instance
> > > of the key was seen) or 'foo.NN' (if multiple instances
> > > of the key were seen).
> > > 
> > > Signed-off-by: Daniel P. Berrange <address@hidden>
> > > ---
> > 
> > If I'm not mistaken, this policy adds a new policy, but all existing
> > clients use the old policy, and the new policy is exercised only by the
> > testsuite additions.  Might be useful to mention that in the commit
> > message, rather than making me read the whole commit before guessing that.
> 
> Correct, this will only be used in combination with the
> later  qdict_crumple method.
> 
> > > +++ b/include/qemu/option.h
> > > @@ -125,7 +125,13 @@ void qemu_opts_set_defaults(QemuOptsList *list, 
> > > const char *params,
> > >                              int permit_abbrev);
> > >  QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
> > >                                 Error **errp);
> > > -QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
> > > +typedef enum {
> > > +    QEMU_OPTS_REPEAT_POLICY_LAST,
> > > +    QEMU_OPTS_REPEAT_POLICY_LIST,
> > 
> > Hmm. I suspect this subtle difference (one vowel) to be the source of
> > typo bugs.  Can we come up with more obvious policy names, such as
> > LAST_ONLY vs. INTO_LIST?  Except that doing that makes it harder to fit
> > 80 columns.  So up to you if you want to ignore me here.
> 
> I'll use POLICY_LAST and POLICY_ALL.
> 
> > ERROR: an occurrence of a duplicate option is considered an error
> 
> Yeah, sure can add that easily.
> 
> > Also, while you turn 'foo=a,foo=b' into 'foo.0=a,foo.1=b', does your
> > code correctly handle the cases of 'foo.0=a,foo=b' and 'foo=a,foo.1=b'?
> > (And what IS the correct handling of those cases logically supposed to be?)
> 
> I'd be inclined to say that is an error. We're only doing this
> hack to maintain compatibility with existing OptsVisitor
> semantics for repeated options, and the scenario you illustrate
> is not possible with OptsVisitor. So I say we keep it an error.
> Either use the old syntax or the new syntax, but not mix them
> ever.

Having looked at this again, I don't think this code should try to
detect 'foo.0=a,foo=b', because qemu_opts_to_qdict() is not placing
an semantic interpretation on the keys.  Such semantics are defined
by the qdict_crumple() method, so that's where I'll have to do such
error detection.

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