Re: [PATCH v2 02/11] keyval: introduce keyval_merge

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This patch introduces a function that merges two keyval-produced
> (or keyval-like) QDicts.  It can be used to emulate the behavior of
> .merge_lists = true QemuOpts groups, merging -readconfig sections and
> command-line options in a single QDict, and also to implement -set.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qemu/option.h    |  1 +
>  tests/unit/test-keyval.c | 58 ++++++++++++++++++++++++++++++++
>  util/keyval.c            | 71 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 130 insertions(+)
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index f73e0dc7d9..d89c66145a 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -149,5 +149,6 @@ QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list);
>  
>  QDict *keyval_parse(const char *params, const char *implied_key,
>                      bool *help, Error **errp);
> +void keyval_merge(QDict *old, const QDict *new, Error **errp);
>  
>  #endif
> diff --git a/tests/unit/test-keyval.c b/tests/unit/test-keyval.c
> index e20c07cf3e..af0581ae6c 100644
> --- a/tests/unit/test-keyval.c
> +++ b/tests/unit/test-keyval.c
> @@ -747,6 +747,61 @@ static void test_keyval_visit_any(void)
>      visit_free(v);
>  }
>  
> +static void test_keyval_merge_dict(void)
> +{
> +    QDict *first = keyval_parse("opt1=abc,opt2.sub1=def,opt2.sub2=ghi,opt3=xyz",
> +                                NULL, NULL, &error_abort);
> +    QDict *second = keyval_parse("opt1=ABC,opt2.sub2=GHI,opt2.sub3=JKL",
> +                                 NULL, NULL, &error_abort);
> +    QDict *combined = keyval_parse("opt1=ABC,opt2.sub1=def,opt2.sub2=GHI,opt2.sub3=JKL,opt3=xyz",
> +                                   NULL, NULL, &error_abort);
> +    Error *err = NULL;
> +
> +    keyval_merge(first, second, &err);
> +    g_assert(!err);
> +    g_assert(qobject_is_equal(QOBJECT(combined), QOBJECT(first)));
> +    qobject_unref(first);
> +    qobject_unref(second);
> +    qobject_unref(combined);
> +}
> +
> +static void test_keyval_merge_list(void)
> +{
> +    QDict *first = keyval_parse("opt1.0=abc,opt2.0=xyz",
> +                                NULL, NULL, &error_abort);
> +    QDict *second = keyval_parse("opt1.0=def",
> +                                 NULL, NULL, &error_abort);
> +    QDict *combined = keyval_parse("opt1.0=abc,opt1.1=def,opt2.0=xyz",
> +                                   NULL, NULL, &error_abort);
> +    Error *err = NULL;
> +
> +    keyval_merge(first, second, &err);
> +    g_assert(!err);
> +    g_assert(qobject_is_equal(QOBJECT(combined), QOBJECT(first)));
> +    qobject_unref(first);
> +    qobject_unref(second);
> +    qobject_unref(combined);
> +}
> +
> +static void test_keyval_merge_conflict(void)
> +{
> +    QDict *first = keyval_parse("opt2=ABC",
> +                                NULL, NULL, &error_abort);
> +    QDict *second = keyval_parse("opt2.sub1=def,opt2.sub2=ghi",
> +                                 NULL, NULL, &error_abort);
> +    QDict *third = qdict_clone_shallow(first);
> +    Error *err = NULL;
> +
> +    keyval_merge(first, second, &err);
> +    error_free_or_abort(&err);
> +    keyval_merge(second, third, &err);
> +    error_free_or_abort(&err);
> +
> +    qobject_unref(first);
> +    qobject_unref(second);
> +    qobject_unref(third);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      g_test_init(&argc, &argv, NULL);
> @@ -760,6 +815,9 @@ int main(int argc, char *argv[])
>      g_test_add_func("/keyval/visit/optional", test_keyval_visit_optional);
>      g_test_add_func("/keyval/visit/alternate", test_keyval_visit_alternate);
>      g_test_add_func("/keyval/visit/any", test_keyval_visit_any);
> +    g_test_add_func("/keyval/merge/dict", test_keyval_merge_dict);
> +    g_test_add_func("/keyval/merge/list", test_keyval_merge_list);
> +    g_test_add_func("/keyval/merge/conflict", test_keyval_merge_conflict);
>      g_test_run();
>      return 0;
>  }
> diff --git a/util/keyval.c b/util/keyval.c
> index be34928813..8006c5df20 100644
> --- a/util/keyval.c
> +++ b/util/keyval.c
> @@ -310,6 +310,77 @@ static char *reassemble_key(GSList *key)
>      return g_string_free(s, FALSE);
>  }
>  
> +/*
> + * Recursive worker for keyval_merge.  @str is the path that led to the
> + * current dictionary, to be used for error messages.  It is modified
> + * internally but restored before the function returns.
> + */

Slight reformatting to better blend in with other comments in this file:

   /*
    * Recursive worker for keyval_merge
    * @str is the path that led to the current dictionary, to be used for
    * error messages.  It is modified internally but restored before the
    * function returns.
    */

> +static void keyval_do_merge(QDict *dest, const QDict *merged, GString *str, Error **errp)
> +{
> +    size_t save_len = str->len;
> +    const QDictEntry *ent;
> +    QObject *old_value;
> +
> +    for (ent = qdict_first(merged); ent; ent = qdict_next(merged, ent)) {
> +        old_value = qdict_get(dest, ent->key);
> +        if (old_value) {
> +            if (qobject_type(old_value) != qobject_type(ent->value)) {
> +                error_setg(errp, "Parameter '%s%s' used inconsistently", str->str, ent->key);

Long line, break after the string literal.

> +                return;
> +            } else if (qobject_type(ent->value) == QTYPE_QDICT) {
> +                /* Merge sub-dictionaries.  */
> +                g_string_append(str, ent->key);
> +                g_string_append_c(str, '.');
> +                keyval_do_merge(qobject_to(QDict, old_value),
> +                                qobject_to(QDict, ent->value),
> +                                str, errp);
> +                g_string_truncate(str, save_len);
> +                continue;
> +            } else if (qobject_type(ent->value) == QTYPE_QLIST) {
> +                /* Append to old list.  */
> +                QList *old = qobject_to(QList, old_value);
> +                QList *new = qobject_to(QList, ent->value);
> +                const QListEntry *item;
> +                QLIST_FOREACH_ENTRY(new, item) {
> +                    qobject_ref(item->value);
> +                    qlist_append_obj(old, item->value);
> +                }
> +                continue;
> +            } else {
> +                assert(qobject_type(ent->value) == QTYPE_QSTRING);
> +            }
> +        }
> +
> +        qobject_ref(ent->value);
> +        qdict_put_obj(dest, ent->key, ent->value);
> +    }
> +}
> +
> +/* Merge the @merged dictionary into @dest.  The dictionaries are expected to be
> + * returned by the keyval parser, and therefore the only expected scalar type
> + * is the * string.  In case the same path is present in both @dest and @merged,
> + * the semantics are as follows:
> + *
> + * - lists are concatenated
> + *
> + * - dictionaries are merged recursively
> + *
> + * - for scalar values, @merged wins
> + *
> + * In case an error is reported, @dest may already have been modified.
> + *
> + * This function can be used to implement semantics analogous to QemuOpts's
> + * .merge_lists = true case, or to implement -set for options backed by QDicts.
> + */

Contents is already lovely.  Now let's tidy up the formatting:

   /*
    * Merge the @merged dictionary into @dest.
    *
    * The dictionaries are expected to be returned by the keyval parser,
    * and therefore the only expected scalar type is the * string.  In
    * case the same path is present in both @dest and @merged, the
    * semantics are as follows:
    *
    * - lists are concatenated
    *
    * - dictionaries are merged recursively
    *
    * - for scalar values, @merged wins
    *
    * In case an error is reported, @dest may already have been modified.
    *
    * This function can be used to implement semantics analogous to
    * QemuOpts's .merge_lists = true case, or to implement -set for
    * options backed by QDicts.
    */

Let's mention where this fails to be analogous.  Perhaps:

    * Note: while QemuOpts is commonly used so that repeated keys
    * overwrite ("last one wins"), it can also be used so that repeated
    * keys build up a list.  keyval_merge() can be analogous to the
    * former usage, but not the latter.

> +void keyval_merge(QDict *dest, const QDict *merged, Error **errp)
> +{
> +    GString *str;
> +
> +    str = g_string_new("");
> +    keyval_do_merge(dest, merged, str, errp);
> +    g_string_free(str, TRUE);
> +}
> +
>  /*
>   * Listify @cur recursively.
>   * Replace QDicts whose keys are all valid list indexes by QLists.

Since I'm asking only for minor improvements:
Reviewed-by: Markus Armbruster <armbru@redhat.com>