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

Forbid ids if the option is intended to be a singleton, as indicated by
list->merge_lists.  This avoids that "./qemu-system-x86_64 -M q35,id=ff"
uses a "pc" machine type.  Instead it errors out.  The affected options
are "qemu-img reopen -o", "qemu-io open -o", -rtc, -M, -boot, -name,
-m, -icount, -smp, -spice.

qemu_opts_create's fail_if_exists parameter is now unnecessary:

- it is unused if id is NULL

- opts_parse only passes false if reached from qemu_opts_set_defaults,
in which case this patch enforces that id must be NULL

- other callers that can pass a non-NULL id always set it to true

Assert that it is true in the only case where "fail_if_exists" matters,
i.e. "id && !lists->merge_lists".  This means that if an id is present,
duplicates are always forbidden, which was already the status quo.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/qemu-option.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

On 09/11/20 17:56, Markus Armbruster wrote:
> Just like any other QemuOptsList, "machine" may have any number of
> QemuOpts.  The ones with non-null ID happen to be ignored silently.
> Known[*] trap for the unwary.
> 
> Are these all singletons?

They are never qemu_opts_find'd with non-NULL id, so I'd say they are.

> If lists>merge_lists, you no longer check id_wellformed().  Easy enough
> to fix: lift the check before this conditional.

Intentional: we always error with INVALID_PARAMETER, so it's pointless 
to check if the id is well-formed.

> After the patch:
> 
>     id        fail_if_exists  merge_lists  |  return
>     non-null  don't care      true         |  fail
>     null      don't care      true         |  existing or else new opts
>     non-null  false           false        |  abort
>     non-null  true            false        |  new opts / fail if exist
>     null      don't care      false        |  new opts
> 
> Still too many 

Discounting the case that aborts as it's not user-controlled (it's 
"just" a matter of inspecting qemu_opts_create callers), the rest can be 
summarized as:

- merge_lists = false: singleton opts with NULL id; non-NULL id fails

- merge_lists = true: always return new opts; non-NULL id fails if dup

> [*] Known to the QemuOpts cognoscenti, whose number could be
> embarrasingly close to one.

Maybe not one, but a single hand certainly has a surplus of fingers.

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 09/11/20 17:56, Markus Armbruster wrote:
>> Just like any other QemuOptsList, "machine" may have any number of
>> QemuOpts.  The ones with non-null ID happen to be ignored silently.
>> Known[*] trap for the unwary.
>> 
>> Are these all singletons?
>
> 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().

>> If lists>merge_lists, you no longer check id_wellformed().  Easy enough
>> to fix: lift the check before this conditional.
>
> Intentional: we always error with INVALID_PARAMETER, so it's pointless 
> to check if the id is well-formed.

My head was about to explode, and then it farted instead.  Sorry fore
the noise!

>> After the patch:
>> 
>>     id        fail_if_exists  merge_lists  |  return
>>     non-null  don't care      true         |  fail
>>     null      don't care      true         |  existing or else new opts
>>     non-null  false           false        |  abort
>>     non-null  true            false        |  new opts / fail if exist
>>     null      don't care      false        |  new opts
>> 
>> Still too many 
>
> Discounting the case that aborts as it's not user-controlled (it's 
> "just" a matter of inspecting qemu_opts_create callers), the rest can be 
> summarized as:
>
> - 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?

>> [*] Known to the QemuOpts cognoscenti, whose number could be
>> embarrasingly close to one.
>
> Maybe not one, but a single hand certainly has a surplus of fingers.
>
> Paolo

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

Reminder of how the affected options are affected:

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

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

qemu_rtc_opts			qemu_find_opts_singleton("rtc")

qemu_machine_opts		qemu_find_opts_singleton("machine")

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

qemu_name_opts			qemu_opts_foreach->parse_name
				parse_name does not use id

qemu_mem_opts			qemu_find_opts_singleton("memory")

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

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

qemu_spice_opts			QTAILQ_FIRST(&qemu_spice_opts.head)

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".

>> - 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.

Paolo