Re: [PATCH v2 4/6] qemu-option: clean up id vs. list->merge_lists

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 09/11/20 19:38, Markus Armbruster wrote:
>>> They are never qemu_opts_find'd with non-NULL id, so I'd say they are.
>> 
>> We also need to check qemu_opts_foreach().
>
> Using qemu_opts_foreach means that e.g. -name id=... was not ignored 
> unlike -M id=....  However, it will be an error now.  We have to check 
> if the callback or its callees use the opt->id

Yes.

> Reminder of how the affected options are affected:

In general, the patch rejects only options that used to be silently
ignored.  As we will see below, there are exceptions where we reject
options that used to work.  Do we want that?

If yes, discuss in commit message and release notes.

More below.

> reopen_opts in qemu-io-cmds.c	qemu_opts_find(&reopen_opts, NULL)

    qopts = qemu_opts_find(&reopen_opts, NULL);
    opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : qdict_new();
    qemu_opts_reset(&reopen_opts);

I guess this could use qemu_find_opts_singleton().  Not sure we want it,
and even if we do, it's not this patch's job.

>
> empty_opts in qemu-io.c		qemu_opts_find(&empty_opts, NULL)

Likewise.

> qemu_rtc_opts			qemu_find_opts_singleton("rtc")
>
> qemu_machine_opts		qemu_find_opts_singleton("machine")

No surprises or funnies here.

> qemu_boot_opts			
> 	QTAILQ_FIRST(&qemu_find_opts("bootopts")->head)

Spelled "boot-opts", and used with a variable spliced on, which defeated
my grep.  It's in hw/nvram/fw_cfg.c and hw/s390x/ipl.c.

vl.c additionally has qemu_opts_find(qemu_find_opts("boot-opts"), NULL).

If the user passes multiple -boot with different ID, we merge the ones
with same ID, and then vl.c gets the (merged) one without ID, but the
other two get the (merged) one that contains the first -boot.  All three
silently ignore the ones they don't get.  Awesomely weird.  I'd call it
a bug.

Test case:

    $ upstream-qemu -S -display none -boot strict=on,id=id -boot strict=off

vl.c uses strict=off, but fw_cfg.c uses strinct=on,id=id.

Outlawing "id" with .merge_lists like this patch does turns the cases
where the two methods yield different results into errors.  This could
break working (if crazy) configurations.  That's okay; I can't see how
the craziness could be fixed without breaking crazy configurations.

I think the commit message should cover this.

I believe we could use qemu_find_opts_singleton() in all three spots.
Not this patch's job.

>
> qemu_name_opts			qemu_opts_foreach->parse_name
> 				parse_name does not use id

First, we use .merge_lists to merge -name with the same ID into a single
QemuOpts, then we use code to merge the resulting QemuOpts, ignoring ID.
Stupid.

Outlawing "id" with .merge_lists like this patch does makes the second
merge a no-op.

This is one of the cases where we reject options that used to work.

If that's wanted, follow-up patch to drop the useless second merge.

If not, unset qemu_name_opts.merge_lists in a separate patch before this
one.

> qemu_mem_opts			qemu_find_opts_singleton("memory")

No surprises or funnies here.

> qemu_icount_opts		qemu_opts_foreach->do_configuree_icount
> 				do_configure_icount->icount_configure
> 				icount_configure does not use id

Same story as for qemu_name_opts.

> qemu_smp_opts
> 	qemu_opts_find(qemu_find_opts("smp-opts"), NULL)

No surprises or funnies here.

> qemu_spice_opts			QTAILQ_FIRST(&qemu_spice_opts.head)

We use the merged one that contains the first -spice, and silently
ignore the rest.  At least we're consistent here.

This is one of the cases where we reject options that used to work.

If that's wanted, follow-up patch to replace the QTAILQ_FIRST() by
something saner.

If not, unset qemu_spice_opts.merge_lists in a separate patch before
this one, and merge like we do for qemu_name_opts.

> To preempt your question, I can add this in the commit message.  Anyway 
> I think it's relatively self-explanatory for most of these that they do 
> not need "id".

Except where they don't need it, but permit it to have an effect anyway.

One of the issues with QemuOpts is that there are too many "clever" ways
to use it.

>>> - merge_lists = false: singleton opts with NULL id; non-NULL id fails
>>
>> Do you mean merge_lists = true here, and ...
>> 
>>> - merge_lists = true: always return new opts; non-NULL id fails if dup
>>
>> ... = false here?
>
> Of course.  1-1 in the brain fart competition.

Hah!