Markus Armbruster <arm...@redhat.com> writes: > "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'. >> >> 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. > > To serve as a replacement for the options visitor, this needs to be able > to behave exactly the same together with a suitably hacked up QObject > input visitor. Before I dive into the actual patch, let me summarize > QemuOpts and options visitor behavior. > > Warning, this is going to get ugly. > > QemuOpts faithfully represents a key=value,... string as a list of > QemuOpt. Each QemuOpt represents one key=value. They are in the same > order. If key occurs multiple times in the string, it occurs just the > same in the list. > > *Except* key "id" is special: it's stored outside the list, and all but > the first one are silently ignored. > > Most users only ever get the last value of a key. Any non-last > key=value are silently ignored. > > We actually exploit this behavior to do defaults, by *prepending* them > to the list. See the use of qemu_opts_set_defaults() in main().
This prepending of defaults assumes all users ignore values other than the last. It breaks if any user gets non-last values. > A few users get all values of keys (other than key "id"): > > * -device, in qdev_device_add() with callback set_property(). > > We first get "driver" and "bus" normally (silently ignoring non-last > values, as usual). All other keys are device properties. To set > them, we get all (key, value), ignore keys "driver" and "bus", and set > the rest. If a key occurs multiple times, it gets set multiple times. > This effectively ignores all but the last one, silently. > > * -semihosting-config, in main() with callback add_semihosting_arg(). > > We first get a bunch of keys normally. Key "arg" is special: it may > be repeated to build a list. To implement that, we get all (key, > value), ignore keys other than "arg", and accumulate the values. > > * -machine & friends, in main() with callback machine_set_property() > > Similar to -device, only for machines, with "type" instead of "driver" > and "bus". > > * -spice, in qemu_spice_init() with callback add_channel() > > Keys "tls-channel" and "plaintext-channel" may be used repeated to > specify multiple channels. To implement that, we get all (key, > value), ignore keys other than "tls-channel" and "plaintext-channel", > and set up a channel for each of the others. > > * -writeconfig, in config_write_opts() with callback config_write_opt() > > We write out all keys in order. > > * The options visitor, in opts_start_struct() > > We convert the list of (key, value) to a hash table of (key, list of > values). Most of the time, the list of values has exactly one > element. > > When the visitor's user asks for a scalar, we return the last element > of the list of values, in lookup_scalar(). > > When the user asks for list elements, we return the elements of the > list of values in order, in opts_next_list(), or if there are none, > the empty list in opts_start_list(). Note that the only way to get non-last values is to iterate over all (key, value). The combination of "getting a specific key's value gets the last one" and "iterating over all keys gets all values" is poor interface design. The latter feature got pressed into service to do list-valued keys. When qemu_opts_set_defaults() got added (commit 4f6dd9a), the bad interaction with the list-valued keys hack wasn't considered, probably because the whole thing had become too byzantine to fully understand. > Unlike the options visitor, this patch (judging from your description) > makes a list only when keys are repeated. The QObject visitor will have > to cope with finding both scalars and lists. When it finds a scalar, > but needs a list, it'll have to wrap it in a list (PATCH 09, I think). > When it finds a list, but needs a scalar, it'll have to fish it out of > the list (where is that?). > >> All existing callers are all converted to explicitly >> request the historical behaviour of only reporting the >> last key. Later patches will make use of the new modes. >> >> Signed-off-by: Daniel P. Berrange <berra...@redhat.com> > > Out of steam for today.