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

[Daniel P. Berrange]

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 <address@hidden>
---
 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);
+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
-- 
2.7.4