Paolo Bonzini <pbonz...@redhat.com> writes:
> 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.
Okay, makes sense.
Do you think working (some of) this into commit message would be worth
your while?
>>> + 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".
Worth testing. But the case "-kernel bc" is also worth testing.
test_qemu_opts_get() tests both:
/* haven't set anything to str2 yet */
opt = qemu_opt_get(opts, "str2");
g_assert(opt == NULL);
qemu_opt_set(opts, "str2", "value", &error_abort);
/* now we have set str2, should know about it */
opt = qemu_opt_get(opts, "str2");
g_assert_cmpstr(opt, ==, "value");
qemu_opt_set(opts, "str2", "value2", &error_abort);
/* having reset the value, the returned should be the reset one */
opt = qemu_opt_get(opts, "str2");
g_assert_cmpstr(opt, ==, "value2");
I'm okay with not improving the test in this patch, or with strictly
extending coverage, preferably in a separate patch that goes before this
one.
