On 4 July 2013 14:09, Markus Armbruster <arm...@redhat.com> 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
