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 exists
On 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
