Patchwork [2/7] qemu-option: Fix qemu_opts_set_defaults() for corner cases

login
register
mail settings
Submitter Markus Armbruster
Date July 4, 2013, 1:09 p.m.
Message ID <1372943363-24081-3-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/256892/
State New
Headers show

Comments

Markus Armbruster - July 4, 2013, 1:09 p.m.
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.

Cc: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/qemu-option.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)
Peter Maydell - July 4, 2013, 2:34 p.m.
On 4 July 2013 14:09, Markus Armbruster <armbru@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
Markus Armbruster - July 4, 2013, 3:28 p.m.
Peter Maydell <peter.maydell@linaro.org> writes:

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

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).
Peter Maydell - July 4, 2013, 3:53 p.m.
On 4 July 2013 16:28, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> 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?

Well, I think if you have a "can be only one" option then
you might as well turn on merging (as you say it's a syntactic
convenience). If you have an option where id= is mandatory
then you could have merging enabled there; but we don't have
any of those. But for options where id= is allowed but not
mandatory then merging doesn't really work. You don't want
 -device e1000 -device megasas
to merge those two, for instance.

So it just seems like it cuts down the set of combinations
to divide it into:
 * can-be-only-one, merges
 * multiple-allowed, no merging

and I guess it's less confusing for users if there aren't
too many different combinations of behaviour.

> QemuOpts has become unmanagably baroque.

Agreed. This is partly why I'm suggesting cutting down
the possible random combinations (especially where we don't
actually use them).

thanks
-- PMM

Patch

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 2715f27..e0ef426 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -914,15 +914,7 @@  static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
         get_opt_value(value, sizeof(value), p+4);
         id = value;
     }
-    if (defaults) {
-        if (!id && !QTAILQ_EMPTY(&list->head)) {
-            opts = qemu_opts_find(list, NULL);
-        } else {
-            opts = qemu_opts_create(list, id, 0, &local_err);
-        }
-    } else {
-        opts = qemu_opts_create(list, id, 1, &local_err);
-    }
+    opts = qemu_opts_create(list, id, !defaults, &local_err);
     if (opts == NULL) {
         if (error_is_set(&local_err)) {
             qerror_report_err(local_err);