Re: [PATCH 01/25] qemu-option: clean up id vs. list->merge_lists

[Paolo Bonzini]

On 19/01/21 14:58, Markus Armbruster wrote:
> > qemu_machine_opts ("-M")
> >         qemu_find_opts_singleton("machine")
>
> Gone since your commit 4988a4ea4d "vl: switch -M parsing to keyval".

Which is part of this series and not yet included in QEMU. Hence the 
commit message talks about it in the present tense.

> 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.  A bug fix of
> sorts.  Should the commit message should cover it?

Yeah, I can add that.

> [qemu_action_opts]
> should not use QemuOpts at all.  Use of qmp_marshal_FOO() is an
> anti-pattern.  Needs cleanup.  Not in this patch, and probably not even
> in this series.

--verbose needed.  Why is it an anti-pattern?  I found it a clever way 
to avoid code duplication. :)  Doesn't matter for this series, anyway.

> > command line is considered.  With this patch we just forbid id
> > on merge-lists QemuOptsLists; if the command line still works,
> > it has the same semantics as before.
>
> It can break working (if weird) command lines, such as ones relying on
> "merge ignoring ID" behavior of -name, -icount, -action.  Okay.

Right, I wrote that down as a feature.  The important thing is keeping 
things the same if they still work.

> [If !lists->merge_lists], if id= is specified, it must be unique,
> i.e. no prior opts with the same id.
>
> Else, we don't check for prior opts without id.
>
> There's at most one opts with a certain id, but there could be any
> number without id.  Is this what we want?

Yes, positively.  Example: "qemu-system-x86_64 -device foo -device bar".

> > Discounting the case that aborts as it's not user-controlled (it's
> > "just" a matter of inspecting qemu_opts_create callers), the paths
> > through qemu_opts_create can be summarized as:
> >
> > - merge_lists = true: singleton opts with NULL id; non-NULL id fails
> >
> > - merge_lists = false: always return new opts; non-NULL id fails if dup
>
> This renders the qemu_opts_foreach() silly.  Cleanup is in order, not
> necessarily in this patch.

Agreed.  This one is already tricky enough (though I like the outcome).

Paolo