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

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Looking at all merge-lists QemuOptsList

outside tests/

>                                          here is how they access their
> QemuOpts:
>
> reopen_opts in qemu-io-cmds.c ("qemu-img reopen -o")
> 	qemu_opts_find(&reopen_opts, NULL)
>
> empty_opts in qemu-io.c ("qemu-io open -o")
> 	qemu_opts_find(&empty_opts, NULL)
>
> qemu_rtc_opts ("-rtc")
> 	qemu_find_opts_singleton("rtc")
>
> qemu_machine_opts ("-M")
> 	qemu_find_opts_singleton("machine")

Gone since your commit 4988a4ea4d "vl: switch -M parsing to keyval".
Did it change behavior when multiple -M are given, with various id=?  I
didn't look at the patch back then, and I'm not going to look at it
now.  Feel free to ignore the question.

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

That one's in hw/nvram/fw_cfg.c and hw/s390x/ipl.c.  However,
softmmu/vl.c uses

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

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

> qemu_name_opts ("-name")
> 	qemu_opts_foreach->parse_name

Funny way to explain qemu_opts_foreach(qemu_name_opts, parse_name, ...)

> 	parse_name does not use id

We first use .merge-lists = true to merge all -name with the same ID,
then we iterate over all the merges.  Combined effect is "merge ignoring
ID".

> qemu_mem_opts ("-m")
> 	qemu_find_opts_singleton("memory")
>
> qemu_icount_opts ("-icount")
> 	qemu_opts_foreach->do_configuree_icount
> 	do_configure_icount->icount_configure
> 	icount_configure does not use id

Just like qemu_name_opts.  My comments apply, plus one:
s/do_configuree_icount/do_configure_icount/.

A recent addition is missing:

  qemu_action_opts ("-action")

Again, just like qemu_name_opts; my comments apply, plus one aside: this
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.

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

That's one access in vl.c.  There's another one that uses
qemu_find_opts_singleton().  Okay, I think.

> qemu_spice_opts ("-spice")
> 	QTAILQ_FIRST(&qemu_spice_opts.head)

There's also machine_opts in qemu-config.c, but that's only to make
query-command-line-options lie backward-compatibly.

> i.e. they don't need an id.  Sometimes its presence is ignored
> (e.g. when using qemu_opts_foreach), sometimes all the options
> with the id are skipped,

(when using qemu_find_opts_singleton() or qemu_opts_find(list, NULL))

>                          sometimes only the first option on the
> command line is considered.

(when using QTAILQ_FIRST)

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

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

Hmm.

If ->merge_lists, id= is forbidden, and all (id-less) opts are merged
into one.

Else, 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?

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

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  util/qemu-option.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index c88e159f18..91f4120ce1 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -619,7 +619,17 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
>  {
>      QemuOpts *opts = NULL;
>  
> -    if (id) {
> +    if (list->merge_lists) {
> +        if (id) {
> +            error_setg(errp, QERR_INVALID_PARAMETER, "id");
> +            return NULL;
> +        }
> +        opts = qemu_opts_find(list, NULL);
> +        if (opts) {
> +            return opts;
> +        }
> +    } else if (id) {
> +        assert(fail_if_exists);
>          if (!id_wellformed(id)) {
>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "id",
>                         "an identifier");
> @@ -629,17 +639,8 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
>          }
>          opts = qemu_opts_find(list, id);
>          if (opts != NULL) {
> -            if (fail_if_exists && !list->merge_lists) {
> -                error_setg(errp, "Duplicate ID '%s' for %s", id, list->name);
> -                return NULL;
> -            } else {
> -                return opts;
> -            }
> -        }
> -    } else if (list->merge_lists) {
> -        opts = qemu_opts_find(list, NULL);
> -        if (opts) {
> -            return opts;
> +            error_setg(errp, "Duplicate ID '%s' for %s", id, list->name);
> +            return NULL;
>          }
>      }
>      opts = g_malloc0(sizeof(*opts));
> @@ -893,7 +894,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
>       * (if unlikely) future misuse:
>       */
>      assert(!defaults || list->merge_lists);
> -    opts = qemu_opts_create(list, id, !defaults, errp);
> +    opts = qemu_opts_create(list, id, !list->merge_lists, errp);
>      g_free(id);
>      if (opts == NULL) {
>          return NULL;

My dread of QemuOpts has been refreshed to its nominal level.