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 | 56 ++++++++++++++++++++++++++++++++++++++++
util/keyval.c | 47 +++++++++++++++++++++++++++++++++
3 files changed, 104 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..254b51e98c 100644
--- a/tests/unit/test-keyval.c
+++ b/tests/unit/test-keyval.c
@@ -747,6 +747,59 @@ static void test_keyval_visit_any(void)
visit_free(v);
}
+static void test_keyval_merge_success(void)
+{
+ QDict *old = keyval_parse("opt1=abc,opt2.sub1=def,opt2.sub2=ghi,opt3=xyz",
+ NULL, NULL, &error_abort);
+ QDict *new = 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(old, new, &err);
+ g_assert(!err);
+ g_assert(qobject_is_equal(QOBJECT(combined), QOBJECT(old)));
+ qobject_unref(old);
+ qobject_unref(new);
+ qobject_unref(combined);
+}
+
+static void test_keyval_merge_list(void)
+{
+ QDict *old = keyval_parse("opt1.0=abc,opt2.0=xyz",
+ NULL, NULL, &error_abort);
+ QDict *new = 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(old, new, &err);
+ g_assert(!err);
+ g_assert(qobject_is_equal(QOBJECT(combined), QOBJECT(old)));
+ qobject_unref(old);
+ qobject_unref(new);
+ qobject_unref(combined);
+}
+
+static void test_keyval_merge_conflict(void)
+{
+ QDict *old = keyval_parse("opt2.sub1=def,opt2.sub2=ghi",
+ NULL, NULL, &error_abort);
+ QDict *new = keyval_parse("opt2=ABC",
+ NULL, NULL, &error_abort);
+ Error *err = NULL;
+
+ keyval_merge(new, old, &err);
+ error_free_or_abort(&err);
+ keyval_merge(old, new, &err);
+ error_free_or_abort(&err);
+
+ qobject_unref(old);
+ qobject_unref(new);
+}
+
int main(int argc, char *argv[])
{
g_test_init(&argc, &argv, NULL);
@@ -760,6 +813,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/success", test_keyval_merge_success);
+ 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..0797f36e1d 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -310,6 +310,53 @@ static char *reassemble_key(GSList *key)
return g_string_free(s, FALSE);
}
+/* Merge two dictionaries. */
+static void keyval_do_merge(QDict *old, const QDict *new, GString *str, Error **errp)
+{
+ size_t save_len = str->len;
+ const QDictEntry *ent;
+ QObject *old_value;
+
+ for (ent = qdict_first(new); ent; ent = qdict_next(new, ent)) {
+ old_value = qdict_get(old, 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);
+ 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;
+ }
+ }
+
+ qobject_ref(ent->value);
+ qdict_put_obj(old, ent->key, ent->value);
+ }
+}
+
+void keyval_merge(QDict *old, const QDict *new, Error **errp)
+{
+ GString *str = g_string_new("");
+ keyval_do_merge(old, new, str, errp);
+ g_string_free(str, TRUE);
+}
+
/*
* Listify @cur recursively.
* Replace QDicts whose keys are all valid list indexes by QLists.
--
2.31.1
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 | 56 ++++++++++++++++++++++++++++++++++++++++
> util/keyval.c | 47 +++++++++++++++++++++++++++++++++
> 3 files changed, 104 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..254b51e98c 100644
> --- a/tests/unit/test-keyval.c
> +++ b/tests/unit/test-keyval.c
> @@ -747,6 +747,59 @@ static void test_keyval_visit_any(void)
> visit_free(v);
> }
>
> +static void test_keyval_merge_success(void)
> +{
> + QDict *old = keyval_parse("opt1=abc,opt2.sub1=def,opt2.sub2=ghi,opt3=xyz",
> + NULL, NULL, &error_abort);
> + QDict *new = 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(old, new, &err);
> + g_assert(!err);
> + g_assert(qobject_is_equal(QOBJECT(combined), QOBJECT(old)));
> + qobject_unref(old);
> + qobject_unref(new);
> + qobject_unref(combined);
> +}
> +
> +static void test_keyval_merge_list(void)
> +{
> + QDict *old = keyval_parse("opt1.0=abc,opt2.0=xyz",
> + NULL, NULL, &error_abort);
> + QDict *new = 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(old, new, &err);
> + g_assert(!err);
> + g_assert(qobject_is_equal(QOBJECT(combined), QOBJECT(old)));
> + qobject_unref(old);
> + qobject_unref(new);
> + qobject_unref(combined);
> +}
This is a success, too. Suggest to name the two positive tests
_merge_dict() and _merge_list().
> +
> +static void test_keyval_merge_conflict(void)
> +{
> + QDict *old = keyval_parse("opt2.sub1=def,opt2.sub2=ghi",
> + NULL, NULL, &error_abort);
> + QDict *new = keyval_parse("opt2=ABC",
> + NULL, NULL, &error_abort);
> + Error *err = NULL;
> +
> + keyval_merge(new, old, &err);
Naming the variables @new and @old us confusing: you pass variable @new
to parameter @old, and @old to @new.
> + error_free_or_abort(&err);
> + keyval_merge(old, new, &err);
> + error_free_or_abort(&err);
Reusing @new is slightly iffy, because keyval_merge() may change its
first argument.
> +
> + qobject_unref(old);
> + qobject_unref(new);
> +}
> +
> int main(int argc, char *argv[])
> {
> g_test_init(&argc, &argv, NULL);
> @@ -760,6 +813,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/success", test_keyval_merge_success);
> + 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..0797f36e1d 100644
> --- a/util/keyval.c
> +++ b/util/keyval.c
> @@ -310,6 +310,53 @@ static char *reassemble_key(GSList *key)
> return g_string_free(s, FALSE);
> }
>
> +/* Merge two dictionaries. */
Okay, but where's the result? Clue: there's no return value, and one of
the dictionaries is const, so the other one must be updated in place.
Also, what's @str for?
> +static void keyval_do_merge(QDict *old, const QDict *new, GString *str, Error **errp)
> +{
> + size_t save_len = str->len;
> + const QDictEntry *ent;
> + QObject *old_value;
> +
> + for (ent = qdict_first(new); ent; ent = qdict_next(new, ent)) {
> + old_value = qdict_get(old, ent->key);
> + if (old_value) {
The two dicts share ent->key, ...
> + if (qobject_type(old_value) != qobject_type(ent->value)) {
> + error_setg(errp, "Parameter '%s%s' used inconsistently", str->str, ent->key);
> + return;
... but the two values cannot be merged. Hmm. See case "overwrite"
below.
> + } 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;
... and both values are dicts: merge them recursively. Good.
> + } 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;
... and both values are lists: concatenate. Good.
> + }
> + }
> +
... and both values are the same other QType (QTYPE_QNULL, QTYPE_QNUM,
QTYPE_QSTRING, QTYPE_QBOOL): overwrite.
Why is overwrite restricted to same QType? Is there no need for
overwriting say a string with a number? Hmm, I guess it's okay, because
keyval_parse() only ever produces QTYPE_QSTRING scalars. May be worth a
comment, preferably in a function contract.
See also the discussion at the end of this message.
> + qobject_ref(ent->value);
> + qdict_put_obj(old, ent->key, ent->value);
> + }
> +}
> +
This function needs a contract. It should spell out the purpose like
the commit message does, because it's rather peculiar.
With a contract in place, simply deleting the helper's function comment
would work for me.
> +void keyval_merge(QDict *old, const QDict *new, Error **errp)
> +{
> + GString *str = g_string_new("");
Humor me: blank line between declarations and statements, please.
> + keyval_do_merge(old, new, str, errp);
> + g_string_free(str, TRUE);
> +}
> +
> /*
> * Listify @cur recursively.
> * Replace QDicts whose keys are all valid list indexes by QLists.
Now let's go back to the stated purpose: 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.
Easy as long as keys are all distinct. Your code does what I'd expect
it to do.
Not so easy when we have multiple mentions of the same key.
QemuOpts fundamentally stores lists of (key, value) pairs.
Most users only ever look at the last such pair added. This provides
"last one wins" semantics.
Example 1: repeated option key overwrites
$ qemu-system-x86_64 -S -display none -monitor stdio -name guest=one,guest=two
QEMU 6.0.50 monitor - type 'help' for more information
(qemu) info name
two
Example 2: even when spread over multiple merge_lists options
$ qemu-system-x86_64 -S -display none -monitor stdio -name guest=one -name guest=two
QEMU 6.0.50 monitor - type 'help' for more information
(qemu) info name
two
Example 3: -set overwrites
$ qemu-system-x86_64 -S -display none -monitor stdio -device ide-cd,id=cd0,serial=one -set device.cd0.serial=two
QEMU 6.0.50 monitor - type 'help' for more information
(qemu) info qtree
bus: main-system-bus
[...]
dev: ide-cd, id "cd0"
[...]
serial = "two"
[...]
However, some users look at *all* pairs. This provides "repeated keys
build up a list" semantics.
Example 4: repeated option key builds a list
$ qemu-system-x86_64 -S -spice tls-port=12345,tls-channel=main,tls-channel=display
This fails for me because I don't have Spice set up (and know basically
nothing about it), but a working variation of it should configure *two*
channels, not one: the second tls-channel= does *not* overwrite the
first one, it configures another channel.
Since -spice is a merge_lists option, this should be the case for
multiple -spice, too. Merging the two options with keyval_merge() would
not preserve that behavior, I'm afraid.
QemuOpts is... complicated.
On 16/06/21 10:38, Markus Armbruster wrote: > > ... and both values are the same other QType (QTYPE_QNULL, QTYPE_QNUM, > QTYPE_QSTRING, QTYPE_QBOOL): overwrite. > > Why is overwrite restricted to same QType? Is there no need for > overwriting say a string with a number? Hmm, I guess it's okay, because > keyval_parse() only ever produces QTYPE_QSTRING scalars. May be worth a > comment, preferably in a function contract. Good point, I can add an assert that the only scalars are QTYPE_QSTRING. > However, some users look at*all* pairs. This provides "repeated keys > build up a list" semantics. > > Example 4: repeated option key builds a list > > $ qemu-system-x86_64 -S -spice tls-port=12345,tls-channel=main,tls-channel=display > > This fails for me because I don't have Spice set up (and know basically > nothing about it), but a working variation of it should configure*two* > channels, not one: the second tls-channel= does*not* overwrite the > first one, it configures another channel. > > Since -spice is a merge_lists option, this should be the case for > multiple -spice, too. Merging the two options with keyval_merge() would > not preserve that behavior, I'm afraid. > > QemuOpts is... complicated. Right, QemuOpts is complicated and keyval a bit less so, which makes keyval not applicable to every case: see -object where we had to resort to the OptsVisitor. For those cases where repeated keys build up a list, it will not be possible to switch them to keyval. The other remarks are mostly cosmetic, so there's not much to say on them. Paolo
© 2016 - 2025 Red Hat, Inc.