qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Daniel P. Berrange
Subject: Re: [Qemu-devel] [PATCH v14 03/19] option: allow qemu_opts_to_qdict to merge repeated options
Date: Wed, 28 Sep 2016 10:35:05 +0100
User-agent: Mutt/1.7.0 (2016-08-17)

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.


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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]