|
From: | Paolo Bonzini |
Subject: | Re: [PATCH v2 3/6] qemu-option: restrict qemu_opts_set to merge-lists QemuOpts |
Date: | Mon, 9 Nov 2020 17:21:34 +0100 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 |
On 09/11/20 16:55, Markus Armbruster wrote:
QemuOptsList *net = qemu_find_opts("net"); - qemu_opts_set(net, NULL, "type", "nic", &error_abort); + qemu_opts_parse(net, "nic", true, &error_abort); #ifdef CONFIG_SLIRP - qemu_opts_set(net, NULL, "type", "user", &error_abort); + qemu_opts_parse(net, "user", true, &error_abort); #endif }Looks safe to me, but I don't quite get why you switch to qemu_opts_parse(). The commit message explains it is "so that qemu_opts_set is now only used on merge-lists QemuOptsList (for which it makes the most sense indeed)..." Is there anything wrong with using ot on non-merge-lists QemuOptsList?
I would *expect* a function named qemu_opts_set to do two things: 1. setting an option in a merge-lists QemuOptsList, such as -kernel. This is indeed what we mostly use qemu_opts_set for.2. setting an option in a non-merge-lists QemuOptsList with non-NULL id, similar to -set.
QEMU does not use qemu_opts_set for the latter (see qemu_set_option) because it wants to use qemu_opts_find rather than qemu_opts_create. In fact it wouldn't *work* to use qemu_opts_set for the latter because qemu_opts_set uses fail_if_exists==1. So:
-> For non-merge-lists QemuOptsList and non-NULL id, it is debatable that qemu_opts_set fails if the (QemuOptsList, id) pair already existsOn the other hand, I would not *expect* qemu_opts_set to create a non-merge-lists QemuOpts with a single option; which it does, though. This leads us directly to:
-> For non-merge-lists QemuOptsList and NULL id, qemu_opts_set hardly adds value over qemu_opts_parse. It does skip some parsing and unescaping, but its two call sites don't really care.So qemu_opts_set has warty behavior for non-merge-lists QemuOptsList if id is non-NULL, and it's mostly pointless if id is NULL. My solution to keeping the API as simple as possible is to limit qemu_opts_set to merge-lists QemuOptsList. For them, it's useful (we don't want comma-unescaping for -kernel) *and* has sane semantics.
+ g_assert(!QTAILQ_EMPTY(&list->head)); + + /* set it again */ + qemu_opts_set(list, "str3", "value", &error_abort); g_assert(!QTAILQ_EMPTY(&list->head));This one not. What are you trying to accomplish?
Improve the testcase, though I should have mentioned it in the commit message. Basically emulating "-kernel bc -kernel def".
Paolo
[Prev in Thread] | Current Thread | [Next in Thread] |