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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6296 bytes --]

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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]