From: Markus Armbruster <armbru@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: berrange@redhat.com, qemu-devel@nongnu.org
Subject: Re: [PATCH 02/11] keyval: introduce keyval_merge
Date: Wed, 16 Jun 2021 10:38:08 +0200 [thread overview]
Message-ID: <87a6nqkxvz.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20210610133538.608390-3-pbonzini@redhat.com> (Paolo Bonzini's message of "Thu, 10 Jun 2021 15:35:29 +0200")
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.
next prev parent reply other threads:[~2021-06-16 8:39 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-10 13:35 [PATCH 00/11] vl: compound properties for machines Paolo Bonzini
2021-06-10 13:35 ` [PATCH 01/11] qom: export more functions for use with non-UserCreatable objects Paolo Bonzini
2021-06-10 13:49 ` Daniel P. Berrangé
2021-06-10 13:35 ` [PATCH 02/11] keyval: introduce keyval_merge Paolo Bonzini
2021-06-16 8:38 ` Markus Armbruster [this message]
2021-06-16 10:49 ` Paolo Bonzini
2021-06-10 13:35 ` [PATCH 03/11] keyval: introduce keyval_parse_into Paolo Bonzini
2021-06-10 13:53 ` Daniel P. Berrangé
2021-06-16 14:09 ` Markus Armbruster
2021-06-16 18:05 ` Paolo Bonzini
2021-06-10 13:35 ` [PATCH 04/11] vl: switch -M parsing to keyval Paolo Bonzini
2021-06-10 13:35 ` [PATCH 05/11] qemu-option: remove now-dead code Paolo Bonzini
2021-06-10 13:54 ` Daniel P. Berrangé
2021-06-10 13:35 ` [PATCH 06/11] machine: move dies from X86MachineState to CpuTopology Paolo Bonzini
2021-06-10 13:55 ` Daniel P. Berrangé
2021-06-10 13:35 ` [PATCH 07/11] machine: move common smp_parse code to caller Paolo Bonzini
2021-06-10 14:34 ` Daniel P. Berrangé
2021-06-10 13:35 ` [PATCH 08/11] machine: add error propagation to mc->smp_parse Paolo Bonzini
2021-06-10 14:35 ` Daniel P. Berrangé
2021-06-10 13:35 ` [PATCH 09/11] machine: pass QAPI struct " Paolo Bonzini
2021-06-10 14:37 ` Daniel P. Berrangé
2021-06-10 13:35 ` [PATCH 10/11] machine: reject -smp dies!=1 for non-PC machines Paolo Bonzini
2021-06-10 14:37 ` Daniel P. Berrangé
2021-06-10 13:35 ` [PATCH 11/11] machine: add smp compound property Paolo Bonzini
2021-06-10 14:41 ` Daniel P. Berrangé
2021-06-14 22:57 ` [PATCH 00/11] vl: compound properties for machines no-reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87a6nqkxvz.fsf@dusky.pond.sub.org \
--to=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).