On Thu, Oct 13, 2016 at 10:31:37AM +0200, Markus Armbruster wrote: > "Daniel P. Berrange" <berra...@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'. > > 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/ :|