Re: [Qemu-devel] [PATCH 2/7] qemu-option: Fix qemu_opts_set_defaults() for corner cases

[Peter Maydell]

On 4 July 2013 14:09, Markus Armbruster <address@hidden> wrote:
> Commit 4f6dd9a changed the initialization of opts in opts_parse() to
> this:
>
>     if (defaults) {
>         if (!id && !QTAILQ_EMPTY(&list->head)) {
>             opts = qemu_opts_find(list, NULL);
>         } else {
>             opts = qemu_opts_create(list, id, 0);
>         }
>     } else {
>         opts = qemu_opts_create(list, id, 1);
>     }
>
> Same as before for !defaults.
>
> If defaults is true, and params has no ID, and options exist, we use
> the first assignment.  It sets opts to null if all options have an ID.
> opts_parse() then returns null.  qemu_opts_set_defaults() asserts the
> value is non-null.  It's the only caller that passes true for
> defaults.
>
> To reproduce, try "-M xenpv -machine id=foo" (yes, "id=foo" is silly,
> but it shouldn't crash).
>
> I believe the function attempts to do the following:
>
>     If options don't yet exist, create new options
>     Else, if defaults, modify the existing options
>     Else, if list->merge_lists, modify the existing options
>     Else, fail
>
> A straightforward call of qemu_opts_create() does exactly that.

I'm not sure this is right. In particular I don't think
that your change will do the right thing if list->merge_list
isn't true (it happens to be true for the only case we
have at the moment that uses qemu_opts_set_defaults()).
If merge_list is false then the old code would prepend
the options to the first entry in the list; with your
change we will instead insert the options as a completely
new entry in the list, which doesn't seem like a sensible
thing to do.

On the other hand I don't think the old code's behaviour
was really right either. I think part of the problem here
is that it really makes no sense to specify id= for a
QemuOptsList with merge_lists=true -- id= is for distinguishing
which of multiple "-whatever id=foo,a=b -whatever id=bar,a=c"
sets you want, whereas merge_lists=true is specifying that
there should only ever be one set, because they're merged.
So I think we should just catch this early and make it
an error. This then means the rest of the code can be
simpler (and prevents the user using id= as a backdoor
for weirdly splitting a single set of options into two).

Next up, does it make sense to use qemu_opts_set_defaults()
on a list without merge_lists set to true? I think the
most sensible semantics here would be that that should mean
"use these defaults for every '-whatever'. So if you set
the defaults for '-whatever' to be 'x=y', then
"-whatever id=foo,a=b -whatever id=bar,a=c" would work
like "-whatever x=y,id=foo,a=b -whatever x=y,id=bar,a=c".
This isn't what the code currently does (what it does do
is I think a historical artefact of the fact that
qemu_opts_set_defaults() predates merge_lists). To implement
it, instead of a single qemu_opts_find() you'd need to
iterate through the list applying the defaults to every
entry.

Or we could just assert() that merge_lists==true for the
moment, with a comment about what the right semantics
would be if anybody actually needed them.

thanks
-- PMM