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:

> Forbid ids if the option is intended to be a singleton, as indicated by
> list->merge_lists.

Well, ->merge_lists need not imply singleton.  Perhaps we only ever use
it that way.  Careful review is called for.

>                     This avoids that "./qemu-system-x86_64 -M q35,id=ff"
> uses a "pc" machine type.

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.

>                            Instead it errors out.  The affected options
> are "qemu-img reopen -o",

reopen_opts in qemu-io-cmds.c

>                           "qemu-io open -o",

empty_opts in qemu-io.c

>                                              -rtc, -M, -boot, -name,
> -m, -icount, -smp,

qemu_rtc_opts, qemu_machine_opts, qemu_boot_opts, qemu_name_opts,
qemu_mem_opts, qemu_icount_opts, qemu_smp_opts in vl.c

>                    -spice.

qemu_spice_opts in spice-core.c.

Are these all singletons?

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

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

Sounds like you're specializing the code (which might be good).

> 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;
> +        }

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

> +    } 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));

Paths through the function before the patch:

    id        fail_if_exists  merge_lists  |  return
    null      don't care      false        |  new opts
    null      don't care      true         |  existing or else new opts
    non-null  false           don't care   |  existing or else new opts
    non-null  true            true         |  existing or else new opts
    non-null  true            false        |  new opts / fail if exist

Too many cases.

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 :)

I'm running out of time for today, and I still need to convince myself
that the changes in behavior are all okay.

> @@ -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;



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