qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 2/7] qemu-option: Fix qemu_opts_set_defaults() for corner cases
Date: Thu, 04 Jul 2013 17:28:01 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux)

Peter Maydell <address@hidden> writes:

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

So my code fails to adhere to my own spec.

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

Isn't interpreting merge_lists as "there can only be one" stretching it
a bit?  All it clearly says to me is "merge multiple options with the
same ID", and that's all the code does.

Merging is merely a syntactic convenience.  Why is that convenience only
justified for "there can only be one" options, such as -machine?

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

I think applying defaults to exactly the options with the same ID as
given in the defaults is defensible semantics.  But debating this would
be a waste of time as long as nobody wants to set defaults with
merge_lists false.

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

QemuOpts has become unmanagably baroque.

My first impulse was to rip out all this default business, and
reimplement its only user completely outside of QemuOpts: dumb down
QEMUMachine default_machine_opts to default_accel, pass it to
configure_accelerator(), and call it a day.  I resisted the temptation.

I doubt fixing qemu_opts_set_defaults() to cover all the bells, whistles
and warts of QemuOpts would be a productive use of our time.  Unless
somebody objects, I'll respin with assert(list->merge_lists).



reply via email to

[Prev in Thread] Current Thread [Next in Thread]