| Message ID | 1372943363-24081-3-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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).
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
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);
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(-)