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

[Paolo Bonzini]

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