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

[Markus Armbruster <armbru@redhat.com>]

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

> 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