Re: [PATCH v2 3/6] qemu-option: restrict qemu_opts_set to merge-lists QemuOpts

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@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.