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 <[email protected]> > > > --- > > > > 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/ :|
