| Message ID | 1475246744-29302-13-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/30/2016 09:45 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=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") I think the last time around you were converting keys (turning "nodes" into "nodes.0"; I think your idea here of keeping a single key tied to QList makes more sense. > > 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. > > 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 <berrange@redhat.com> > --- > -QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict); > +typedef enum { > + /* Last occurrence of a duplicate option silently replaces earlier */ > + QEMU_OPTS_REPEAT_POLICY_LAST, > + /* Each occurrence of a duplicate option converts value to a QList */ > + QEMU_OPTS_REPEAT_POLICY_ALL, > + /* First occurrence of a duplicate option causes an error */ > + QEMU_OPTS_REPEAT_POLICY_ERROR, > +} QemuOptsRepeatPolicy; > + Nicer. Thanks! > +++ b/tests/test-qemu-opts.c > @@ -421,6 +421,130 @@ static void test_qemu_opts_set(void) > g_assert(opts == NULL); > } > > +static void test_qemu_opts_to_qdict_repeat_all(void) > +{ > + QemuOpts *opts; > + QDict *dict; > + QList *list; > + const QListEntry *ent; > + QString *str; > + > + /* dynamically initialized (parsed) opts */ > + opts = qemu_opts_parse(&opts_list_03, > + "size=1024,nodes=10,nodes=4-5,nodes=1-2,policy=bind", > + false, NULL); > + g_assert(opts); > + > + dict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_ALL, > + &error_abort); > + g_assert(dict); > + > + g_assert_cmpstr(qdict_get_str(dict, "size"), ==, "1024"); > + g_assert(qdict_haskey(dict, "nodes")); > + list = qobject_to_qlist(qdict_get(dict, "nodes")); > + g_assert(list); > + > + ent = qlist_first(list); > + g_assert(ent); > + str = qobject_to_qstring(ent->value); > + g_assert(str); > + g_assert_cmpstr(qstring_get_str(str), ==, "10"); > + > + ent = qlist_next(ent); > + g_assert(ent); > + str = qobject_to_qstring(ent->value); > + g_assert(str); > + g_assert_cmpstr(qstring_get_str(str), ==, "4-5"); > + > + ent = qlist_next(ent); > + g_assert(ent); > + str = qobject_to_qstring(ent->value); > + g_assert(str); > + g_assert_cmpstr(qstring_get_str(str), ==, "1-2"); Yes, this is different from v13, but does look nicer with the value turning into QList instead of the key being rewritten into dotted form. > +++ b/util/qemu-option.c > @@ -1057,23 +1057,73 @@ void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp) > * The QDict values are of type QString. > * TODO We'll want to use types appropriate for opt->desc->type, but > * this is enough for now. > + * > + * If the @opts contains multiple occurrences of the same key, > + * then the @repeatPolicy parameter determines how they are to > + * be handled. Traditional behaviour was to only store the > + * last occurrence, but if @repeatPolicy is set to > + * QEMU_OPTS_REPEAT_POLICY_ALL, then a QList will be returned > + * containing all values, for any key with multiple occurrences. > + * The QEMU_OPTS_REPEAT_POLICY_ERROR value can be used to fail > + * conversion with an error if multiple occurrences of a key > + * are seen. > */ > -QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict) > +QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict, > + QemuOptsRepeatPolicy repeatPolicy, > + Error **errp) Reviewed-by: Eric Blake <eblake@redhat.com>
"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'. > > 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(). 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(). 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 <berrange@redhat.com> Out of steam for today.
"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? > 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 <berrange@redhat.com> > --- > blockdev.c | 7 ++- > include/qemu/option.h | 13 ++++- > monitor.c | 3 +- > qapi/qobject-input-visitor.c | 4 +- > qemu-img.c | 4 +- > qemu-io-cmds.c | 3 +- > qemu-io.c | 6 +- > qemu-nbd.c | 3 +- > qom/object_interfaces.c | 3 +- > tests/test-qemu-opts.c | 132 +++++++++++++++++++++++++++++++++++++++++++ > tests/test-replication.c | 9 ++- > util/qemu-option.c | 64 ++++++++++++++++++--- > 12 files changed, 229 insertions(+), 22 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 814d49f..a999d46 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -914,7 +914,8 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) > > /* Get a QDict for processing the options */ > bs_opts = qdict_new(); > - qemu_opts_to_qdict(all_opts, bs_opts); > + qemu_opts_to_qdict(all_opts, bs_opts, QEMU_OPTS_REPEAT_POLICY_LAST, > + &error_abort); > > legacy_opts = qemu_opts_create(&qemu_legacy_drive_opts, NULL, 0, > &error_abort); > @@ -3804,8 +3805,8 @@ void hmp_drive_add_node(Monitor *mon, const char *optstr) > return; > } > > - qdict = qemu_opts_to_qdict(opts, NULL); > - > + qdict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, > + &error_abort); > if (!qdict_get_try_str(qdict, "node-name")) { > QDECREF(qdict); > error_report("'node-name' needs to be specified"); > diff --git a/include/qemu/option.h b/include/qemu/option.h > index 29f3f18..ffd841d 100644 > --- a/include/qemu/option.h > +++ b/include/qemu/option.h > @@ -125,7 +125,18 @@ 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(const QemuOpts *opts, QDict *qdict); Blank line here, please. > +typedef enum { > + /* Last occurrence of a duplicate option silently replaces earlier */ > + QEMU_OPTS_REPEAT_POLICY_LAST, > + /* Each occurrence of a duplicate option converts value to a QList */ > + QEMU_OPTS_REPEAT_POLICY_ALL, > + /* First occurrence of a duplicate option causes an error */ > + QEMU_OPTS_REPEAT_POLICY_ERROR, > +} QemuOptsRepeatPolicy; > + > +QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict, > + QemuOptsRepeatPolicy repeatPolicy, > + Error **errp); > void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp); > > typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts *opts, Error **errp); > diff --git a/monitor.c b/monitor.c > index 14089cc..84e79d4 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -2642,7 +2642,8 @@ static QDict *monitor_parse_arguments(Monitor *mon, > if (!opts) { > goto fail; > } > - qemu_opts_to_qdict(opts, qdict); > + qemu_opts_to_qdict(opts, qdict, QEMU_OPTS_REPEAT_POLICY_LAST, > + &error_abort); > qemu_opts_del(opts); > } > break; > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c > index 6b3d0f2..2287d11 100644 > --- a/qapi/qobject-input-visitor.c > +++ b/qapi/qobject-input-visitor.c > @@ -759,7 +759,9 @@ Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts, > QObject *pobj = NULL; > Visitor *v = NULL; > > - pdict = qemu_opts_to_qdict(opts, NULL); > + pdict = qemu_opts_to_qdict(opts, NULL, > + QEMU_OPTS_REPEAT_POLICY_LAST, Join the previous two lines, please. > + errp); Unlike the other callers, this one doesn't pass &error_abort. The error handling is actually dead code. Let's pass &error_abort and be done with it. > if (!pdict) { > goto cleanup; > } > diff --git a/qemu-img.c b/qemu-img.c > index 851422a..f47ea75 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -273,7 +273,9 @@ static BlockBackend *img_open_opts(const char *optstr, > QDict *options; > Error *local_err = NULL; > BlockBackend *blk; > - options = qemu_opts_to_qdict(opts, NULL); > + options = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, > + &error_abort); > + > blk = blk_new_open(NULL, NULL, options, flags, &local_err); > if (!blk) { > error_reportf_err(local_err, "Could not open '%s': ", optstr); > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c > index 3a3838a..ce654f4 100644 > --- a/qemu-io-cmds.c > +++ b/qemu-io-cmds.c > @@ -1952,7 +1952,8 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv) > } > > qopts = qemu_opts_find(&reopen_opts, NULL); > - opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL; > + opts = qopts ? qemu_opts_to_qdict(qopts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, > + &error_abort) : NULL; Short ternaries are more legible than if statements, but this one is pushing the limit now. > qemu_opts_reset(&reopen_opts); > > brq = bdrv_reopen_queue(NULL, bs, opts, flags); > diff --git a/qemu-io.c b/qemu-io.c > index db129ea..bb374a6 100644 > --- a/qemu-io.c > +++ b/qemu-io.c > @@ -207,7 +207,8 @@ static int open_f(BlockBackend *blk, int argc, char **argv) > } > > qopts = qemu_opts_find(&empty_opts, NULL); > - opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL; > + opts = qopts ? qemu_opts_to_qdict(qopts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, > + &error_abort) : NULL; Likewise. > qemu_opts_reset(&empty_opts); > > if (optind == argc - 1) { > @@ -593,7 +594,8 @@ int main(int argc, char **argv) > if (!qopts) { > exit(1); > } > - opts = qemu_opts_to_qdict(qopts, NULL); > + opts = qemu_opts_to_qdict(qopts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, > + &error_abort); Even this line's getting ugly. Can we find a prefix that's less loquacious than QEMU_OPTS_REPEAT_POLICY_, but still self-explaining? > openfile(NULL, flags, writethrough, opts); > } else { > if (format) { > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 99297a5..73b40b1 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -859,7 +859,8 @@ int main(int argc, char **argv) > qemu_opts_reset(&file_opts); > exit(EXIT_FAILURE); > } > - options = qemu_opts_to_qdict(opts, NULL); > + options = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, > + &error_abort); > qemu_opts_reset(&file_opts); > blk = blk_new_open(NULL, NULL, options, flags, &local_err); > } else { > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c > index ded4d84..fdc406b 100644 > --- a/qom/object_interfaces.c > +++ b/qom/object_interfaces.c > @@ -161,7 +161,8 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error **errp) > Object *obj = NULL; > > v = opts_visitor_new(opts); > - pdict = qemu_opts_to_qdict(opts, NULL); > + pdict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, > + &error_abort); > > obj = user_creatable_add(pdict, v, errp); > visit_free(v); > diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c > index a505a3e..3b59978 100644 > --- a/tests/test-qemu-opts.c > +++ b/tests/test-qemu-opts.c > @@ -421,6 +421,130 @@ static void test_qemu_opts_set(void) > g_assert(opts == NULL); > } > > + > +static void test_qemu_opts_to_qdict_repeat_last(void) > +{ > + QemuOpts *opts; > + QDict *dict; > + > + /* dynamically initialized (parsed) opts */ What are you trying to say with this comment? Hmm, you merely copied it from the existing tests. I'd drop the existing instances of this comment instead. > + opts = qemu_opts_parse(&opts_list_03, > + "size=1024,nodes=10,nodes=4-5,nodes=1-2,policy=bind", > + false, NULL); > + g_assert(opts); You could pass &error_abort. > + > + dict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, > + &error_abort); > + g_assert(dict); > + > + One blank line, not two, please. > + g_assert_cmpstr(qdict_get_str(dict, "size"), ==, "1024"); > + g_assert_cmpstr(qdict_get_str(dict, "nodes"), ==, "1-2"); > + g_assert(!qdict_haskey(dict, "nodes.0")); > + g_assert(!qdict_haskey(dict, "nodes.1")); > + g_assert(!qdict_haskey(dict, "nodes.2")); > + g_assert_cmpstr(qdict_get_str(dict, "policy"), ==, "bind"); Checking for absence of "nodes.0"... feels odd. A better idea is to check we got exactly the keys we expect, and no more. Here's a simple way to do that: g_assert(qdict_size(dict) == 3); > + QDECREF(dict); > + > + qemu_opts_del(opts); > + qemu_opts_reset(&opts_list_03); > +} > + > + > +static void test_qemu_opts_to_qdict_repeat_all(void) > +{ > + QemuOpts *opts; > + QDict *dict; > + QList *list; > + const QListEntry *ent; > + QString *str; > + > + /* dynamically initialized (parsed) opts */ > + opts = qemu_opts_parse(&opts_list_03, > + "size=1024,nodes=10,nodes=4-5,nodes=1-2,policy=bind", > + false, NULL); > + g_assert(opts); > + > + dict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_ALL, > + &error_abort); > + g_assert(dict); > + > + g_assert_cmpstr(qdict_get_str(dict, "size"), ==, "1024"); > + g_assert(qdict_haskey(dict, "nodes")); > + list = qobject_to_qlist(qdict_get(dict, "nodes")); > + g_assert(list); Works, but I'd do obj = qdict_get(dict, "nodes"); g_assert(obj); list = qobject_to_qlist(obj); g_assert(list); > + > + ent = qlist_first(list); > + g_assert(ent); > + str = qobject_to_qstring(ent->value); > + g_assert(str); > + g_assert_cmpstr(qstring_get_str(str), ==, "10"); Since g_assert_cmpstr() does the right thing when @str is null, g_assert(str) is redundant. More of the same below. > + > + ent = qlist_next(ent); > + g_assert(ent); > + str = qobject_to_qstring(ent->value); > + g_assert(str); > + g_assert_cmpstr(qstring_get_str(str), ==, "4-5"); > + > + ent = qlist_next(ent); > + g_assert(ent); > + str = qobject_to_qstring(ent->value); > + g_assert(str); > + g_assert_cmpstr(qstring_get_str(str), ==, "1-2"); > + > + ent = qlist_next(ent); > + g_assert(!ent); > + > + g_assert_cmpstr(qdict_get_str(dict, "policy"), ==, "bind"); g_assert(qdict_size(dict) == 3); > + QDECREF(dict); > + > + qemu_opts_del(opts); > + qemu_opts_reset(&opts_list_03); > +} > + > +static void test_qemu_opts_to_qdict_repeat_err_fail(void) > +{ > + QemuOpts *opts; > + QDict *dict; > + Error *err = NULL; > + > + /* dynamically initialized (parsed) opts */ > + opts = qemu_opts_parse(&opts_list_03, > + "size=1024,nodes=10,nodes=4-5,nodes=1-2,policy=bind", > + false, NULL); > + g_assert(opts); > + > + dict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_ERROR, > + &err); > + error_free_or_abort(&err); > + g_assert(!dict); > + > + qemu_opts_del(opts); > + qemu_opts_reset(&opts_list_03); > +} > + > +static void test_qemu_opts_to_qdict_repeat_err_ok(void) > +{ > + QemuOpts *opts; > + QDict *dict; > + > + /* dynamically initialized (parsed) opts */ > + opts = qemu_opts_parse(&opts_list_03, > + "size=1024,nodes=10,policy=bind", > + false, NULL); > + g_assert(opts); > + > + dict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_ERROR, > + &error_abort); > + g_assert_cmpstr(qdict_get_str(dict, "size"), ==, "1024"); > + g_assert_cmpstr(qdict_get_str(dict, "nodes"), ==, "10"); > + g_assert_cmpstr(qdict_get_str(dict, "policy"), ==, "bind"); g_assert(qdict_size(dict) == 3); > + > + QDECREF(dict); > + qemu_opts_del(opts); > + qemu_opts_reset(&opts_list_03); > +} > + > int main(int argc, char *argv[]) > { > register_opts(); > @@ -435,6 +559,14 @@ int main(int argc, char *argv[]) > g_test_add_func("/qemu-opts/opt_unset", test_qemu_opt_unset); > g_test_add_func("/qemu-opts/opts_reset", test_qemu_opts_reset); > g_test_add_func("/qemu-opts/opts_set", test_qemu_opts_set); > + g_test_add_func("/qemu-opts/to_qdict/repeat-last", > + test_qemu_opts_to_qdict_repeat_last); > + g_test_add_func("/qemu-opts/to_qdict/repeat-all", > + test_qemu_opts_to_qdict_repeat_all); > + g_test_add_func("/qemu-opts/to_qdict/repeat-err-fail", > + test_qemu_opts_to_qdict_repeat_err_fail); > + g_test_add_func("/qemu-opts/to_qdict/repeat-err-ok", > + test_qemu_opts_to_qdict_repeat_err_ok); > g_test_run(); > return 0; > } > diff --git a/tests/test-replication.c b/tests/test-replication.c > index 0997bd8..e267f9a 100644 > --- a/tests/test-replication.c > +++ b/tests/test-replication.c > @@ -181,7 +181,8 @@ static BlockBackend *start_primary(void) > opts = qemu_opts_parse_noisily(&qemu_drive_opts, cmdline, false); > g_free(cmdline); > > - qdict = qemu_opts_to_qdict(opts, NULL); > + qdict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, > + &error_abort); > qdict_set_default_str(qdict, BDRV_OPT_CACHE_DIRECT, "off"); > qdict_set_default_str(qdict, BDRV_OPT_CACHE_NO_FLUSH, "off"); > > @@ -311,7 +312,8 @@ static BlockBackend *start_secondary(void) > opts = qemu_opts_parse_noisily(&qemu_drive_opts, cmdline, false); > g_free(cmdline); > > - qdict = qemu_opts_to_qdict(opts, NULL); > + qdict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, > + &error_abort); > qdict_set_default_str(qdict, BDRV_OPT_CACHE_DIRECT, "off"); > qdict_set_default_str(qdict, BDRV_OPT_CACHE_NO_FLUSH, "off"); > > @@ -336,7 +338,8 @@ static BlockBackend *start_secondary(void) > opts = qemu_opts_parse_noisily(&qemu_drive_opts, cmdline, false); > g_free(cmdline); > > - qdict = qemu_opts_to_qdict(opts, NULL); > + qdict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, > + &error_abort); > qdict_set_default_str(qdict, BDRV_OPT_CACHE_DIRECT, "off"); > qdict_set_default_str(qdict, BDRV_OPT_CACHE_NO_FLUSH, "off"); > > diff --git a/util/qemu-option.c b/util/qemu-option.c > index 418cbb6..0129ede 100644 > --- a/util/qemu-option.c > +++ b/util/qemu-option.c > @@ -1057,23 +1057,73 @@ void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp) > * The QDict values are of type QString. > * TODO We'll want to use types appropriate for opt->desc->type, but > * this is enough for now. While there, we could document @obj and @dict properly. But see below on @dict. > + * > + * If the @opts contains multiple occurrences of the same key, > + * then the @repeatPolicy parameter determines how they are to > + * be handled. Traditional behaviour was to only store the > + * last occurrence, but if @repeatPolicy is set to > + * QEMU_OPTS_REPEAT_POLICY_ALL, then a QList will be returned > + * containing all values, for any key with multiple occurrences. > + * The QEMU_OPTS_REPEAT_POLICY_ERROR value can be used to fail > + * conversion with an error if multiple occurrences of a key > + * are seen. What the traditional behavior was is quite interesting in a commit message. Less so in a comment. Suggest: * be handled. With @repeatPolicy QEMU_OPTS_REPEAT_POLICY_LAST, * all values but the last are silently ignored. With * QEMU_OPTS_REPEAT_POLICY_ALL, a QList will be returned * containing all values, for any key with multiple occurrences. * With QEMU_OPTS_REPEAT_POLICY_ERROR, the conversion fails. Let's spell out that the function can only fail with QEMU_OPTS_REPEAT_POLICY_ERROR: * This is the only way this function can fail. > */ > -QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict) > +QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict, > + QemuOptsRepeatPolicy repeatPolicy, > + Error **errp) > { > QemuOpt *opt; > - QObject *val; > + QObject *val, *prevval; I'd prefer prev_val. > + QList *list; > + QDict *ret; > > - if (!qdict) { > - qdict = qdict_new(); > + if (qdict) { > + ret = qdict; > + } else { > + ret = qdict_new(); > } You need this change to get the reference counting on failure right. This makes me question the qdict parameter. I can see two non-null arguments for it outside tests. The one in drive_new() is basically stupid: bs_opts = qdict_new(); qemu_opts_to_qdict(all_opts, bs_opts, QEMU_OPTS_REPEAT_POLICY_LAST, &error_abort); Easily replaced by bs_opts = qemu_opts_to_qdict(all_opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, &error_abort); The one in monitor_parse_arguments() qemu_opts_to_qdict(opts, qdict, QEMU_OPTS_REPEAT_POLICY_LAST, &error_abort); could be replaced by QDECREF(qdict); qdict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, &error_abort); at a negligible loss of efficiency. We could then drop the parameter, simplifying the function's contract. Let's insert a blank line here... > if (opts->id) { > - qdict_put(qdict, "id", qstring_from_str(opts->id)); > + qdict_put(ret, "id", qstring_from_str(opts->id)); > } ... and here. > 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? > + 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. > } > - return qdict; > + return ret; > } > > /* Validate parsed opts against descriptions where no
Markus Armbruster <armbru@redhat.com> writes: > "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'. >> >> 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 <berrange@redhat.com> > > Out of steam for today.
On Wed, Oct 12, 2016 at 07:46:00PM +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'. > > > > 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(). > > 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(). > > 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?). If my code finds a list but wants a scalar, it is reporting an error. Regards, Daniel
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. > > 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
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Wed, Oct 12, 2016 at 07:46:00PM +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'. >> > >> > 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. [...] >> 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?). > > If my code finds a list but wants a scalar, it is reporting an > error. I'm afraid that breaks things like --numa node,cpus=0,cpus=2,mem=512,nodeid=0,nodeid=0 The options visitor inteprets cpus=0,cpus=2 as a list [0,2] (because NumaNodeOptions member @cpus has a list type) and nodeid=0,nodid=0 as integer 0 (because member @nodeid has a scalar type).
On Fri, Oct 21, 2016 at 01:09:07PM +0200, Markus Armbruster wrote: > "Daniel P. Berrange" <berrange@redhat.com> writes: > > > On Wed, Oct 12, 2016 at 07:46:00PM +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'. > >> > > >> > 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. > [...] > >> 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?). > > > > If my code finds a list but wants a scalar, it is reporting an > > error. > > I'm afraid that breaks things like > > --numa node,cpus=0,cpus=2,mem=512,nodeid=0,nodeid=0 > > The options visitor inteprets cpus=0,cpus=2 as a list [0,2] (because > NumaNodeOptions member @cpus has a list type) and nodeid=0,nodid=0 as > integer 0 (because member @nodeid has a scalar type). Right, but that command line syntax is arguably broken already - the app/user invoking QEMU is buggy, so I felt it was justifiable to change from silently ignoring the app error to telling them of their mistake. Regards, Daniel
"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. [...]
diff --git a/blockdev.c b/blockdev.c index 814d49f..a999d46 100644 --- a/blockdev.c +++ b/blockdev.c @@ -914,7 +914,8 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) /* Get a QDict for processing the options */ bs_opts = qdict_new(); - qemu_opts_to_qdict(all_opts, bs_opts); + qemu_opts_to_qdict(all_opts, bs_opts, QEMU_OPTS_REPEAT_POLICY_LAST, + &error_abort); legacy_opts = qemu_opts_create(&qemu_legacy_drive_opts, NULL, 0, &error_abort); @@ -3804,8 +3805,8 @@ void hmp_drive_add_node(Monitor *mon, const char *optstr) return; } - qdict = qemu_opts_to_qdict(opts, NULL); - + qdict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, + &error_abort); if (!qdict_get_try_str(qdict, "node-name")) { QDECREF(qdict); error_report("'node-name' needs to be specified"); diff --git a/include/qemu/option.h b/include/qemu/option.h index 29f3f18..ffd841d 100644 --- a/include/qemu/option.h +++ b/include/qemu/option.h @@ -125,7 +125,18 @@ 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(const QemuOpts *opts, QDict *qdict); +typedef enum { + /* Last occurrence of a duplicate option silently replaces earlier */ + QEMU_OPTS_REPEAT_POLICY_LAST, + /* Each occurrence of a duplicate option converts value to a QList */ + QEMU_OPTS_REPEAT_POLICY_ALL, + /* First occurrence of a duplicate option causes an error */ + QEMU_OPTS_REPEAT_POLICY_ERROR, +} QemuOptsRepeatPolicy; + +QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict, + QemuOptsRepeatPolicy repeatPolicy, + Error **errp); void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp); typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts *opts, Error **errp); diff --git a/monitor.c b/monitor.c index 14089cc..84e79d4 100644 --- a/monitor.c +++ b/monitor.c @@ -2642,7 +2642,8 @@ static QDict *monitor_parse_arguments(Monitor *mon, if (!opts) { goto fail; } - qemu_opts_to_qdict(opts, qdict); + qemu_opts_to_qdict(opts, qdict, QEMU_OPTS_REPEAT_POLICY_LAST, + &error_abort); qemu_opts_del(opts); } break; diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c index 6b3d0f2..2287d11 100644 --- a/qapi/qobject-input-visitor.c +++ b/qapi/qobject-input-visitor.c @@ -759,7 +759,9 @@ Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts, QObject *pobj = NULL; Visitor *v = NULL; - pdict = qemu_opts_to_qdict(opts, NULL); + pdict = qemu_opts_to_qdict(opts, NULL, + QEMU_OPTS_REPEAT_POLICY_LAST, + errp); if (!pdict) { goto cleanup; } diff --git a/qemu-img.c b/qemu-img.c index 851422a..f47ea75 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -273,7 +273,9 @@ static BlockBackend *img_open_opts(const char *optstr, QDict *options; Error *local_err = NULL; BlockBackend *blk; - options = qemu_opts_to_qdict(opts, NULL); + options = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, + &error_abort); + blk = blk_new_open(NULL, NULL, options, flags, &local_err); if (!blk) { error_reportf_err(local_err, "Could not open '%s': ", optstr); diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index 3a3838a..ce654f4 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -1952,7 +1952,8 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv) } qopts = qemu_opts_find(&reopen_opts, NULL); - opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL; + opts = qopts ? qemu_opts_to_qdict(qopts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, + &error_abort) : NULL; qemu_opts_reset(&reopen_opts); brq = bdrv_reopen_queue(NULL, bs, opts, flags); diff --git a/qemu-io.c b/qemu-io.c index db129ea..bb374a6 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -207,7 +207,8 @@ static int open_f(BlockBackend *blk, int argc, char **argv) } qopts = qemu_opts_find(&empty_opts, NULL); - opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL; + opts = qopts ? qemu_opts_to_qdict(qopts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, + &error_abort) : NULL; qemu_opts_reset(&empty_opts); if (optind == argc - 1) { @@ -593,7 +594,8 @@ int main(int argc, char **argv) if (!qopts) { exit(1); } - opts = qemu_opts_to_qdict(qopts, NULL); + opts = qemu_opts_to_qdict(qopts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, + &error_abort); openfile(NULL, flags, writethrough, opts); } else { if (format) { diff --git a/qemu-nbd.c b/qemu-nbd.c index 99297a5..73b40b1 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -859,7 +859,8 @@ int main(int argc, char **argv) qemu_opts_reset(&file_opts); exit(EXIT_FAILURE); } - options = qemu_opts_to_qdict(opts, NULL); + options = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, + &error_abort); qemu_opts_reset(&file_opts); blk = blk_new_open(NULL, NULL, options, flags, &local_err); } else { diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c index ded4d84..fdc406b 100644 --- a/qom/object_interfaces.c +++ b/qom/object_interfaces.c @@ -161,7 +161,8 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error **errp) Object *obj = NULL; v = opts_visitor_new(opts); - pdict = qemu_opts_to_qdict(opts, NULL); + pdict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, + &error_abort); obj = user_creatable_add(pdict, v, errp); visit_free(v); diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c index a505a3e..3b59978 100644 --- a/tests/test-qemu-opts.c +++ b/tests/test-qemu-opts.c @@ -421,6 +421,130 @@ static void test_qemu_opts_set(void) g_assert(opts == NULL); } + +static void test_qemu_opts_to_qdict_repeat_last(void) +{ + QemuOpts *opts; + QDict *dict; + + /* dynamically initialized (parsed) opts */ + opts = qemu_opts_parse(&opts_list_03, + "size=1024,nodes=10,nodes=4-5,nodes=1-2,policy=bind", + false, NULL); + g_assert(opts); + + dict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, + &error_abort); + g_assert(dict); + + + g_assert_cmpstr(qdict_get_str(dict, "size"), ==, "1024"); + g_assert_cmpstr(qdict_get_str(dict, "nodes"), ==, "1-2"); + g_assert(!qdict_haskey(dict, "nodes.0")); + g_assert(!qdict_haskey(dict, "nodes.1")); + g_assert(!qdict_haskey(dict, "nodes.2")); + g_assert_cmpstr(qdict_get_str(dict, "policy"), ==, "bind"); + QDECREF(dict); + + qemu_opts_del(opts); + qemu_opts_reset(&opts_list_03); +} + + +static void test_qemu_opts_to_qdict_repeat_all(void) +{ + QemuOpts *opts; + QDict *dict; + QList *list; + const QListEntry *ent; + QString *str; + + /* dynamically initialized (parsed) opts */ + opts = qemu_opts_parse(&opts_list_03, + "size=1024,nodes=10,nodes=4-5,nodes=1-2,policy=bind", + false, NULL); + g_assert(opts); + + dict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_ALL, + &error_abort); + g_assert(dict); + + g_assert_cmpstr(qdict_get_str(dict, "size"), ==, "1024"); + g_assert(qdict_haskey(dict, "nodes")); + list = qobject_to_qlist(qdict_get(dict, "nodes")); + g_assert(list); + + ent = qlist_first(list); + g_assert(ent); + str = qobject_to_qstring(ent->value); + g_assert(str); + g_assert_cmpstr(qstring_get_str(str), ==, "10"); + + ent = qlist_next(ent); + g_assert(ent); + str = qobject_to_qstring(ent->value); + g_assert(str); + g_assert_cmpstr(qstring_get_str(str), ==, "4-5"); + + ent = qlist_next(ent); + g_assert(ent); + str = qobject_to_qstring(ent->value); + g_assert(str); + g_assert_cmpstr(qstring_get_str(str), ==, "1-2"); + + ent = qlist_next(ent); + g_assert(!ent); + + g_assert_cmpstr(qdict_get_str(dict, "policy"), ==, "bind"); + QDECREF(dict); + + qemu_opts_del(opts); + qemu_opts_reset(&opts_list_03); +} + +static void test_qemu_opts_to_qdict_repeat_err_fail(void) +{ + QemuOpts *opts; + QDict *dict; + Error *err = NULL; + + /* dynamically initialized (parsed) opts */ + opts = qemu_opts_parse(&opts_list_03, + "size=1024,nodes=10,nodes=4-5,nodes=1-2,policy=bind", + false, NULL); + g_assert(opts); + + dict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_ERROR, + &err); + error_free_or_abort(&err); + g_assert(!dict); + + qemu_opts_del(opts); + qemu_opts_reset(&opts_list_03); +} + +static void test_qemu_opts_to_qdict_repeat_err_ok(void) +{ + QemuOpts *opts; + QDict *dict; + + /* dynamically initialized (parsed) opts */ + opts = qemu_opts_parse(&opts_list_03, + "size=1024,nodes=10,policy=bind", + false, NULL); + g_assert(opts); + + dict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_ERROR, + &error_abort); + g_assert_cmpstr(qdict_get_str(dict, "size"), ==, "1024"); + g_assert_cmpstr(qdict_get_str(dict, "nodes"), ==, "10"); + g_assert_cmpstr(qdict_get_str(dict, "policy"), ==, "bind"); + + QDECREF(dict); + qemu_opts_del(opts); + qemu_opts_reset(&opts_list_03); +} + int main(int argc, char *argv[]) { register_opts(); @@ -435,6 +559,14 @@ int main(int argc, char *argv[]) g_test_add_func("/qemu-opts/opt_unset", test_qemu_opt_unset); g_test_add_func("/qemu-opts/opts_reset", test_qemu_opts_reset); g_test_add_func("/qemu-opts/opts_set", test_qemu_opts_set); + g_test_add_func("/qemu-opts/to_qdict/repeat-last", + test_qemu_opts_to_qdict_repeat_last); + g_test_add_func("/qemu-opts/to_qdict/repeat-all", + test_qemu_opts_to_qdict_repeat_all); + g_test_add_func("/qemu-opts/to_qdict/repeat-err-fail", + test_qemu_opts_to_qdict_repeat_err_fail); + g_test_add_func("/qemu-opts/to_qdict/repeat-err-ok", + test_qemu_opts_to_qdict_repeat_err_ok); g_test_run(); return 0; } diff --git a/tests/test-replication.c b/tests/test-replication.c index 0997bd8..e267f9a 100644 --- a/tests/test-replication.c +++ b/tests/test-replication.c @@ -181,7 +181,8 @@ static BlockBackend *start_primary(void) opts = qemu_opts_parse_noisily(&qemu_drive_opts, cmdline, false); g_free(cmdline); - qdict = qemu_opts_to_qdict(opts, NULL); + qdict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, + &error_abort); qdict_set_default_str(qdict, BDRV_OPT_CACHE_DIRECT, "off"); qdict_set_default_str(qdict, BDRV_OPT_CACHE_NO_FLUSH, "off"); @@ -311,7 +312,8 @@ static BlockBackend *start_secondary(void) opts = qemu_opts_parse_noisily(&qemu_drive_opts, cmdline, false); g_free(cmdline); - qdict = qemu_opts_to_qdict(opts, NULL); + qdict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, + &error_abort); qdict_set_default_str(qdict, BDRV_OPT_CACHE_DIRECT, "off"); qdict_set_default_str(qdict, BDRV_OPT_CACHE_NO_FLUSH, "off"); @@ -336,7 +338,8 @@ static BlockBackend *start_secondary(void) opts = qemu_opts_parse_noisily(&qemu_drive_opts, cmdline, false); g_free(cmdline); - qdict = qemu_opts_to_qdict(opts, NULL); + qdict = qemu_opts_to_qdict(opts, NULL, QEMU_OPTS_REPEAT_POLICY_LAST, + &error_abort); qdict_set_default_str(qdict, BDRV_OPT_CACHE_DIRECT, "off"); qdict_set_default_str(qdict, BDRV_OPT_CACHE_NO_FLUSH, "off"); diff --git a/util/qemu-option.c b/util/qemu-option.c index 418cbb6..0129ede 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -1057,23 +1057,73 @@ void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp) * The QDict values are of type QString. * TODO We'll want to use types appropriate for opt->desc->type, but * this is enough for now. + * + * If the @opts contains multiple occurrences of the same key, + * then the @repeatPolicy parameter determines how they are to + * be handled. Traditional behaviour was to only store the + * last occurrence, but if @repeatPolicy is set to + * QEMU_OPTS_REPEAT_POLICY_ALL, then a QList will be returned + * containing all values, for any key with multiple occurrences. + * The QEMU_OPTS_REPEAT_POLICY_ERROR value can be used to fail + * conversion with an error if multiple occurrences of a key + * are seen. */ -QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict) +QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict, + QemuOptsRepeatPolicy repeatPolicy, + Error **errp) { QemuOpt *opt; - QObject *val; + QObject *val, *prevval; + QList *list; + QDict *ret; - if (!qdict) { - qdict = qdict_new(); + if (qdict) { + ret = qdict; + } else { + ret = qdict_new(); } if (opts->id) { - qdict_put(qdict, "id", qstring_from_str(opts->id)); + qdict_put(ret, "id", qstring_from_str(opts->id)); } 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); + 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); + } } - return qdict; + return ret; } /* Validate parsed opts against descriptions where no
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. 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 <berrange@redhat.com> --- blockdev.c | 7 ++- include/qemu/option.h | 13 ++++- monitor.c | 3 +- qapi/qobject-input-visitor.c | 4 +- qemu-img.c | 4 +- qemu-io-cmds.c | 3 +- qemu-io.c | 6 +- qemu-nbd.c | 3 +- qom/object_interfaces.c | 3 +- tests/test-qemu-opts.c | 132 +++++++++++++++++++++++++++++++++++++++++++ tests/test-replication.c | 9 ++- util/qemu-option.c | 64 ++++++++++++++++++--- 12 files changed, 229 insertions(+), 22 deletions(-)