| Message ID | 1474982001-20878-4-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/27/2016 08:13 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=1024 > nodes=1-2 > policy=bind > > This adds the ability for the caller to ask that the > repeated keys be turned into list indexes: > > size=1024 > nodes.0=10 > nodes.1=4-5 > nodes.2=1-2 > policy=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 'foo' (if only a single instance > of the key was seen) or 'foo.NN' (if multiple instances > of the key were seen). > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- If I'm not mistaken, this policy adds a new policy, but all existing clients use the old policy, and the new policy is exercised only by the testsuite additions. Might be useful to mention that in the commit message, rather than making me read the whole commit before guessing that. > +++ b/blockdev.c > @@ -911,7 +911,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); git send-email/format-patch -O/path/to/file (or the corresponding config option) allows you to sort the diff to put the interesting stuff first (in this case, the new enum). > +++ b/include/qemu/option.h > @@ -125,7 +125,13 @@ 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(QemuOpts *opts, QDict *qdict); > +typedef enum { > + QEMU_OPTS_REPEAT_POLICY_LAST, > + QEMU_OPTS_REPEAT_POLICY_LIST, Hmm. I suspect this subtle difference (one vowel) to be the source of typo bugs. Can we come up with more obvious policy names, such as LAST_ONLY vs. INTO_LIST? Except that doing that makes it harder to fit 80 columns. So up to you if you want to ignore me here. On the other hand, a documentation comment here would go a long ways to helping future readers: LAST: last occurrence of a duplicate option silently overwrites all earlier LIST: each occurrence of a duplicate option converts it into a list maybe you also want to add: ERROR: an occurrence of a duplicate option is considered an error Also, while you turn 'foo=a,foo=b' into 'foo.0=a,foo.1=b', does your code correctly handle the cases of 'foo.0=a,foo=b' and 'foo=a,foo.1=b'? (And what IS the correct handling of those cases logically supposed to be?) > +++ b/tests/test-qemu-opts.c > @@ -421,6 +421,45 @@ static void test_qemu_opts_set(void) > g_assert(opts == NULL); > } > > + > +static void test_qemu_opts_to_qdict(void) > +{ Here would be a good place to test the two mixed-use optstrings I mentioned above (inconsistent use of plain vs. list syntax). > +} > + > int main(int argc, char *argv[]) > { > +++ b/util/qemu-option.c > @@ -1058,10 +1058,12 @@ void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp) > * TODO We'll want to use types appropriate for opt->desc->type, but > * this is enough for now. > */ > -QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict) > +QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict, > + QemuOptsRepeatPolicy repeatPolicy) > { > QemuOpt *opt; > - QObject *val; > + QObject *val, *prevval; > + QDict *lists = qdict_new(); > > if (!qdict) { > qdict = qdict_new(); > @@ -1070,9 +1072,42 @@ QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict) > qdict_put(qdict, "id", qstring_from_str(opts->id)); > } > QTAILQ_FOREACH(opt, &opts->head, next) { > + gchar *key = NULL; > val = QOBJECT(qstring_from_str(opt->str)); > - qdict_put_obj(qdict, opt->name, val); > + switch (repeatPolicy) { > + case QEMU_OPTS_REPEAT_POLICY_LIST: > + if (qdict_haskey(lists, opt->name)) { > + /* Current val goes into 'foo.N' */ > + int64_t max = qdict_get_int(lists, opt->name); > + max++; > + key = g_strdup_printf("%s.%" PRId64, opt->name, max); > + qdict_put_obj(lists, opt->name, QOBJECT(qint_from_int(max))); > + qdict_put_obj(qdict, key, val); > + } else if (qdict_haskey(qdict, opt->name)) { > + /* Move previous val from 'foo' to 'foo.0' */ > + prevval = qdict_get(qdict, opt->name); > + qobject_incref(prevval); > + qdict_del(qdict, opt->name); > + key = g_strdup_printf("%s.0", opt->name); > + qdict_put_obj(qdict, key, prevval); > + g_free(key); > + > + /* Current val goes into 'foo.1' */ > + key = g_strdup_printf("%s.1", opt->name); > + qdict_put_obj(lists, opt->name, QOBJECT(qint_from_int(1))); > + qdict_put_obj(qdict, key, val); > + } else { > + qdict_put_obj(qdict, key ? key : opt->name, val); Dead ?: here; key is always NULL. > + } > + break; > + > + case QEMU_OPTS_REPEAT_POLICY_LAST: > + qdict_put_obj(qdict, key ? key : opt->name, val); Likewise. > + break; > + } > + g_free(key); > } > + QDECREF(lists); > return qdict; > } > > Overall, I like the idea.
On Tue, Sep 27, 2016 at 05:03:17PM -0500, Eric Blake wrote: > On 09/27/2016 08:13 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=1024 > > nodes=1-2 > > policy=bind > > > > This adds the ability for the caller to ask that the > > repeated keys be turned into list indexes: > > > > size=1024 > > nodes.0=10 > > nodes.1=4-5 > > nodes.2=1-2 > > policy=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 'foo' (if only a single instance > > of the key was seen) or 'foo.NN' (if multiple instances > > of the key were seen). > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > If I'm not mistaken, this policy adds a new policy, but all existing > clients use the old policy, and the new policy is exercised only by the > testsuite additions. Might be useful to mention that in the commit > message, rather than making me read the whole commit before guessing that. Correct, this will only be used in combination with the later qdict_crumple method. > > +++ b/include/qemu/option.h > > @@ -125,7 +125,13 @@ 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(QemuOpts *opts, QDict *qdict); > > +typedef enum { > > + QEMU_OPTS_REPEAT_POLICY_LAST, > > + QEMU_OPTS_REPEAT_POLICY_LIST, > > Hmm. I suspect this subtle difference (one vowel) to be the source of > typo bugs. Can we come up with more obvious policy names, such as > LAST_ONLY vs. INTO_LIST? Except that doing that makes it harder to fit > 80 columns. So up to you if you want to ignore me here. I'll use POLICY_LAST and POLICY_ALL. > ERROR: an occurrence of a duplicate option is considered an error Yeah, sure can add that easily. > Also, while you turn 'foo=a,foo=b' into 'foo.0=a,foo.1=b', does your > code correctly handle the cases of 'foo.0=a,foo=b' and 'foo=a,foo.1=b'? > (And what IS the correct handling of those cases logically supposed to be?) I'd be inclined to say that is an error. We're only doing this hack to maintain compatibility with existing OptsVisitor semantics for repeated options, and the scenario you illustrate is not possible with OptsVisitor. So I say we keep it an error. Either use the old syntax or the new syntax, but not mix them ever. Regards, Daniel
On Wed, Sep 28, 2016 at 10:35:05AM +0100, Daniel P. Berrange wrote: > On Tue, Sep 27, 2016 at 05:03:17PM -0500, Eric Blake wrote: > > On 09/27/2016 08:13 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=1024 > > > nodes=1-2 > > > policy=bind > > > > > > This adds the ability for the caller to ask that the > > > repeated keys be turned into list indexes: > > > > > > size=1024 > > > nodes.0=10 > > > nodes.1=4-5 > > > nodes.2=1-2 > > > policy=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 'foo' (if only a single instance > > > of the key was seen) or 'foo.NN' (if multiple instances > > > of the key were seen). > > > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > > --- > > > > If I'm not mistaken, this policy adds a new policy, but all existing > > clients use the old policy, and the new policy is exercised only by the > > testsuite additions. Might be useful to mention that in the commit > > message, rather than making me read the whole commit before guessing that. > > Correct, this will only be used in combination with the > later qdict_crumple method. > > > > +++ b/include/qemu/option.h > > > @@ -125,7 +125,13 @@ 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(QemuOpts *opts, QDict *qdict); > > > +typedef enum { > > > + QEMU_OPTS_REPEAT_POLICY_LAST, > > > + QEMU_OPTS_REPEAT_POLICY_LIST, > > > > Hmm. I suspect this subtle difference (one vowel) to be the source of > > typo bugs. Can we come up with more obvious policy names, such as > > LAST_ONLY vs. INTO_LIST? Except that doing that makes it harder to fit > > 80 columns. So up to you if you want to ignore me here. > > I'll use POLICY_LAST and POLICY_ALL. > > > ERROR: an occurrence of a duplicate option is considered an error > > Yeah, sure can add that easily. > > > Also, while you turn 'foo=a,foo=b' into 'foo.0=a,foo.1=b', does your > > code correctly handle the cases of 'foo.0=a,foo=b' and 'foo=a,foo.1=b'? > > (And what IS the correct handling of those cases logically supposed to be?) > > I'd be inclined to say that is an error. We're only doing this > hack to maintain compatibility with existing OptsVisitor > semantics for repeated options, and the scenario you illustrate > is not possible with OptsVisitor. So I say we keep it an error. > Either use the old syntax or the new syntax, but not mix them > ever. Having looked at this again, I don't think this code should try to detect 'foo.0=a,foo=b', because qemu_opts_to_qdict() is not placing an semantic interpretation on the keys. Such semantics are defined by the qdict_crumple() method, so that's where I'll have to do such error detection. Regards, Daniel
diff --git a/blockdev.c b/blockdev.c index 3010393..5ef3193 100644 --- a/blockdev.c +++ b/blockdev.c @@ -911,7 +911,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); legacy_opts = qemu_opts_create(&qemu_legacy_drive_opts, NULL, 0, &error_abort); @@ -3758,8 +3759,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); 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 2a5266f..328c468 100644 --- a/include/qemu/option.h +++ b/include/qemu/option.h @@ -125,7 +125,13 @@ 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(QemuOpts *opts, QDict *qdict); +typedef enum { + QEMU_OPTS_REPEAT_POLICY_LAST, + QEMU_OPTS_REPEAT_POLICY_LIST, +} QemuOptsRepeatPolicy; + +QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict, + QemuOptsRepeatPolicy repeatPolicy); 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 83c4edf..7dcd66b 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); qemu_opts_del(opts); } break; diff --git a/qemu-img.c b/qemu-img.c index ceffefe..b399ae5 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); + 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..e14beed 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) : 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..b295afa 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) : 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); openfile(NULL, flags, writethrough, opts); } else { if (format) { diff --git a/qemu-nbd.c b/qemu-nbd.c index 99297a5..54eb3ce 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); 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 bf59846..48edf2f 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); 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..b68ef74 100644 --- a/tests/test-qemu-opts.c +++ b/tests/test-qemu-opts.c @@ -421,6 +421,45 @@ static void test_qemu_opts_set(void) g_assert(opts == NULL); } + +static void test_qemu_opts_to_qdict(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); + 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); + + dict = qemu_opts_to_qdict(opts, NULL, + QEMU_OPTS_REPEAT_POLICY_LIST); + g_assert(dict); + + g_assert_cmpstr(qdict_get_str(dict, "size"), ==, "1024"); + g_assert(!qdict_haskey(dict, "nodes")); + g_assert_cmpstr(qdict_get_str(dict, "nodes.0"), ==, "10"); + g_assert_cmpstr(qdict_get_str(dict, "nodes.1"), ==, "4-5"); + g_assert_cmpstr(qdict_get_str(dict, "nodes.2"), ==, "1-2"); + g_assert_cmpstr(qdict_get_str(dict, "policy"), ==, "bind"); + QDECREF(dict); + + qemu_opts_del(opts); +} + int main(int argc, char *argv[]) { register_opts(); @@ -435,6 +474,7 @@ 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", test_qemu_opts_to_qdict); g_test_run(); return 0; } diff --git a/tests/test-replication.c b/tests/test-replication.c index 0997bd8..6165ac9 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); 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); 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); 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 41b356c..ad28d4e 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -1058,10 +1058,12 @@ void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp) * TODO We'll want to use types appropriate for opt->desc->type, but * this is enough for now. */ -QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict) +QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict, + QemuOptsRepeatPolicy repeatPolicy) { QemuOpt *opt; - QObject *val; + QObject *val, *prevval; + QDict *lists = qdict_new(); if (!qdict) { qdict = qdict_new(); @@ -1070,9 +1072,42 @@ QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict) qdict_put(qdict, "id", qstring_from_str(opts->id)); } QTAILQ_FOREACH(opt, &opts->head, next) { + gchar *key = NULL; val = QOBJECT(qstring_from_str(opt->str)); - qdict_put_obj(qdict, opt->name, val); + switch (repeatPolicy) { + case QEMU_OPTS_REPEAT_POLICY_LIST: + if (qdict_haskey(lists, opt->name)) { + /* Current val goes into 'foo.N' */ + int64_t max = qdict_get_int(lists, opt->name); + max++; + key = g_strdup_printf("%s.%" PRId64, opt->name, max); + qdict_put_obj(lists, opt->name, QOBJECT(qint_from_int(max))); + qdict_put_obj(qdict, key, val); + } else if (qdict_haskey(qdict, opt->name)) { + /* Move previous val from 'foo' to 'foo.0' */ + prevval = qdict_get(qdict, opt->name); + qobject_incref(prevval); + qdict_del(qdict, opt->name); + key = g_strdup_printf("%s.0", opt->name); + qdict_put_obj(qdict, key, prevval); + g_free(key); + + /* Current val goes into 'foo.1' */ + key = g_strdup_printf("%s.1", opt->name); + qdict_put_obj(lists, opt->name, QOBJECT(qint_from_int(1))); + qdict_put_obj(qdict, key, val); + } else { + qdict_put_obj(qdict, key ? key : opt->name, val); + } + break; + + case QEMU_OPTS_REPEAT_POLICY_LAST: + qdict_put_obj(qdict, key ? key : opt->name, val); + break; + } + g_free(key); } + QDECREF(lists); return qdict; }
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=1024 nodes=1-2 policy=bind This adds the ability for the caller to ask that the repeated keys be turned into list indexes: size=1024 nodes.0=10 nodes.1=4-5 nodes.2=1-2 policy=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 'foo' (if only a single instance of the key was seen) or 'foo.NN' (if multiple instances of the key were seen). Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- blockdev.c | 7 ++++--- include/qemu/option.h | 8 +++++++- monitor.c | 3 ++- 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 | 40 ++++++++++++++++++++++++++++++++++++++++ tests/test-replication.c | 9 ++++++--- util/qemu-option.c | 41 ++++++++++++++++++++++++++++++++++++++--- 11 files changed, 110 insertions(+), 17 deletions(-)