Paolo Bonzini <pbonz...@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 <pbonz...@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 <arm...@redhat.com>
