[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v14 12/21] option: allow qemu_opts_to_qdict to m
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH v14 12/21] option: allow qemu_opts_to_qdict to merge repeated options |
Date: |
Thu, 20 Oct 2016 15:37:04 +0100 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
On Thu, Oct 13, 2016 at 10:31:37AM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange" <address@hidden> 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'.
>
> Actually three cases, not two:
>
> 0. qdict does not contain the key means empty list.
>
> 1. qdict contains the key with a QString value means list of one
> element.
>
> 2. qdict contains the key with a QList value means list of more than one
> element.
>
> I consider this weird. However, it's usefully weird with at least your
> QObject input visitor.
>
> > 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.
>
> Got users for this policy in the pipeline?
I in fact used it in the QObjectInputVisitor, when the
autocreate_list is not set.
I guess strictly speaking this is not back-compatible
if someone is passing repeated keys, but I judged that
rather than silently ignoring this incorrect usage, it
was acceptable to report an error.
> > QTAILQ_FOREACH(opt, &opts->head, next) {
> > val = QOBJECT(qstring_from_str(opt->str));
> > - qdict_put_obj(qdict, opt->name, val);
> > +
> > + if (qdict_haskey(ret, opt->name)) {
> > + switch (repeatPolicy) {
> > + case QEMU_OPTS_REPEAT_POLICY_ERROR:
> > + error_setg(errp, "Option '%s' appears more than once",
> > + opt->name);
> > + qobject_decref(val);
> > + if (!qdict) {
> > + QDECREF(ret);
> > + }
> > + return NULL;
> > +
> > + case QEMU_OPTS_REPEAT_POLICY_ALL:
> > + prevval = qdict_get(ret, opt->name);
> > + if (qobject_type(prevval) == QTYPE_QLIST) {
> > + /* Already identified this key as a list */
> > + list = qobject_to_qlist(prevval);
> > + } else {
> > + /* Replace current scalar with a list */
> > + list = qlist_new();
> > + qobject_incref(prevval);
>
> Where is this reference decremented?
'prevval' is a borrowed reference from 'ret', against the
key opt->name.
qdict_put_obj decrements the reference we borrowed
from ret against the key opt->name.
qlist_append_obj() takes ownership of the reference
it is passed, so we must qobject_incref() to avoid
qdict_put_obj free'ing prevval.
When we call qdict_put_obj() we're replacing the value
currently associted
>
> > + qlist_append_obj(list, prevval);
> > + qdict_put_obj(ret, opt->name, QOBJECT(list));
> > + }
> > + qlist_append_obj(list, val);
> > + break;
> > +
> > + case QEMU_OPTS_REPEAT_POLICY_LAST:
> > + /* Just discard previously set value */
> > + qdict_put_obj(ret, opt->name, val);
> > + break;
> > + }
> > + } else {
> > + qdict_put_obj(ret, opt->name, val);
> > + }
>
> A possible alternative:
>
> val = QOBJECT(qstring_from_str(opt->str));
>
> if (qdict_haskey(ret, opt->name)) {
> switch (repeatPolicy) {
> case QEMU_OPTS_REPEAT_POLICY_ERROR:
> error_setg(errp, "Option '%s' appears more than once",
> opt->name);
> qobject_decref(val);
> if (!qdict) {
> QDECREF(ret);
> }
> return NULL;
>
> case QEMU_OPTS_REPEAT_POLICY_ALL:
> prevval = qdict_get(ret, opt->name);
> if (qobject_type(prevval) == QTYPE_QLIST) {
> /* Already identified this key as a list */
> list = qobject_to_qlist(prevval);
> } else {
> /* Replace current scalar with a list */
> list = qlist_new();
> qobject_incref(prevval);
> qlist_append_obj(list, prevval);
> }
> qlist_append_obj(list, val);
> val = QOBJECT(list);
> break;
>
> case QEMU_OPTS_REPEAT_POLICY_LAST:
> break;
> }
> }
> qdict_put_obj(ret, opt->name, val);
>
> This shows the common part of the behavior more clearly. Matter of
> taste, you get to use your artistic license.
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/ :|