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>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org