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

[Markus Armbruster <armbru@redhat.com>]

"Daniel P. Berrange" <berrange@redhat.com> writes:

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

While usage like

    -machine usb=on,usb=off

could perhaps be declared erroneous post hoc, the "last one wins"
feature also has genuinely useful applications.  For instance, tack a
correction to a long command line:

    -machine usb=on [... lots of other stuff ...] -machine usb=off

Or modify canned configuration:

    -readconfig vm1.config -machine usb=off
    # vm1.config sets usb=on, -machine overrides

    -readconfig vm1.config -set drive.disk0.file=tmp.qcow2
    # vm1.config defines drive "disk0", -set overrides its image file

I don't think we can break them all.

[...]