| Message ID | 1496899257-25800-1-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
Thomas Huth <thuth@redhat.com> writes: > Commit bde4d9205 ("Fix the -accel parameter and the documentation for > 'hax'") introduced a regression by adding a new local accel_opts > variable which shadows the variable with the same name that is > declared at the beginning of the main() scope. This causes the > qemu_tcg_configure() call later to be always called with NULL, so > that the thread=xxx option gets ignored. Fix it by removing the > local accel_opts variable and use "opts" instead, which is meant > for storing temporary QemuOpts values. > And while we're at it, also change the exit(1) here to exit(0) > since asking for help is not an error. > > Fixes: bde4d9205ee9def98852ff6054cdef4efd74e1f8 > Reported-by: Markus Armbruster <armbru@redhat.com> > Reported-by: Emilio G. Cota <cota@braap.org> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > vl.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/vl.c b/vl.c > index be4dcf2..5aba544 100644 > --- a/vl.c > +++ b/vl.c > @@ -3757,21 +3757,18 @@ int main(int argc, char **argv, char **envp) > qdev_prop_register_global(&kvm_pit_lost_tick_policy); > break; > } > - case QEMU_OPTION_accel: { > - QemuOpts *accel_opts; > - > + case QEMU_OPTION_accel: > accel_opts = qemu_opts_parse_noisily(qemu_find_opts("accel"), > optarg, true); > optarg = qemu_opt_get(accel_opts, "accel"); > if (!optarg || is_help_option(optarg)) { > error_printf("Possible accelerators: kvm, xen, hax, tcg\n"); > - exit(1); > + exit(0); > } > - accel_opts = qemu_opts_create(qemu_find_opts("machine"), NULL, > - false, &error_abort); > - qemu_opt_set(accel_opts, "accel", optarg, &error_abort); > + opts = qemu_opts_create(qemu_find_opts("machine"), NULL, > + false, &error_abort); > + qemu_opt_set(opts, "accel", optarg, &error_abort); > break; > - } > case QEMU_OPTION_usb: > olist = qemu_find_opts("machine"); > qemu_opts_parse_noisily(olist, "usb=on", false); The fix is fine, so Reviewed-by: Markus Armbruster <armbru@redhat.com> The code could use further cleanup, however: * I hate how we use accel_opts. It effectively caches the value of qemu_accel_opts' "active" QemuOpts, to be passed to qemu_tcg_configure() later. Two reasons: - Action at a distance: to understand the call qemu_tcg_configure(accel_opts, &error_fatal), you have to trace execution backwards to find the last assignment to accel_opts. - QemuOpts is odd, and this interacts with one of its oddities in a less than obvious way: qemu_accel_opts has .merge_lists set. Because of that, repeated -accel with the same @id get merged into a single QemuOpts. Example: -machine pc -machine graphics=off combines with defaults into a single QemuOpts firmware=bios-256k.bin,accel=kvm,usb=on,type=pc,graphics=off Example: -machine pc -machine graphics=off,id=bad-idea results in *two* QemuOpts, one without @id firmware=bios-256k.bin,accel=kvm,usb=on,type=pc and one with id=bad-idea, which gets silently ignored. Example: -name Peter -name Paul results in a single QemuOpts guest=Peter. Example: -name Peter -name Paul,id=bad-idea results in two QemuOpts guest=Peter and guest=Paul,id=bad-idea. We use *both*, and the resulting guest name is actually Paul. We suck. Example: -accel=kvm -accel=tcg results in a single QemuOpts accel=tcg Example: -accel=kvm -accel=tcg,id=bad-idea results in a two QemuOpts, and we use whichever comes last. Subtly different from using both. We always find new ways to suck. Option group "boot-opts", "memory", "smp-opts" behave like "machine", I think. Option group "icount" behaves like "name" (same action at a distance anti-pattern). * The list of accelerators in qemu-options.hx is approaching a length where sorting may make sense. You decide. * Assigning to optarg is not nice. It should be assigned to only once per iteration, via lookup_opt(). Even more pointless abuse in case QEMU_OPTION_rotate. That one should be converted to qemu_strtol(). * The error reporting for invalid accelerator is ugly, e.g. for -accel xxx, we get: "xxx" accelerator not found. No accelerator found! Want one error message, without the over-excited '!'.
Thomas Huth <thuth@redhat.com> writes: > Commit bde4d9205 ("Fix the -accel parameter and the documentation for > 'hax'") introduced a regression by adding a new local accel_opts > variable which shadows the variable with the same name that is > declared at the beginning of the main() scope. This causes the > qemu_tcg_configure() call later to be always called with NULL, so > that the thread=xxx option gets ignored. Fix it by removing the > local accel_opts variable and use "opts" instead, which is meant > for storing temporary QemuOpts values. > And while we're at it, also change the exit(1) here to exit(0) > since asking for help is not an error. > > Fixes: bde4d9205ee9def98852ff6054cdef4efd74e1f8 > Reported-by: Markus Armbruster <armbru@redhat.com> > Reported-by: Emilio G. Cota <cota@braap.org> > Signed-off-by: Thomas Huth <thuth@redhat.com> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Tested-by: Alex Bennée <alex.bennee@linaro.org> I'll leave the wider question of a better layout to the QemuOpts experts (I was/am very much an amateur when I first added the thread option). > --- > vl.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/vl.c b/vl.c > index be4dcf2..5aba544 100644 > --- a/vl.c > +++ b/vl.c > @@ -3757,21 +3757,18 @@ int main(int argc, char **argv, char **envp) > qdev_prop_register_global(&kvm_pit_lost_tick_policy); > break; > } > - case QEMU_OPTION_accel: { > - QemuOpts *accel_opts; > - > + case QEMU_OPTION_accel: > accel_opts = qemu_opts_parse_noisily(qemu_find_opts("accel"), > optarg, true); > optarg = qemu_opt_get(accel_opts, "accel"); > if (!optarg || is_help_option(optarg)) { > error_printf("Possible accelerators: kvm, xen, hax, tcg\n"); > - exit(1); > + exit(0); > } > - accel_opts = qemu_opts_create(qemu_find_opts("machine"), NULL, > - false, &error_abort); > - qemu_opt_set(accel_opts, "accel", optarg, &error_abort); > + opts = qemu_opts_create(qemu_find_opts("machine"), NULL, > + false, &error_abort); > + qemu_opt_set(opts, "accel", optarg, &error_abort); > break; > - } > case QEMU_OPTION_usb: > olist = qemu_find_opts("machine"); > qemu_opts_parse_noisily(olist, "usb=on", false); -- Alex Bennée
On 08/06/2017 09:04, Markus Armbruster wrote: > The fix is fine, so > > Reviewed-by: Markus Armbruster <armbru@redhat.com> Who's going to merge it? Paolo
On 08.06.2017 14:53, Paolo Bonzini wrote: > > > On 08/06/2017 09:04, Markus Armbruster wrote: >> The fix is fine, so >> >> Reviewed-by: Markus Armbruster <armbru@redhat.com> > > Who's going to merge it? Could you do it, Paolo? You also merged my commit bde4d9205 that introduced this bug... Thomas
diff --git a/vl.c b/vl.c index be4dcf2..5aba544 100644 --- a/vl.c +++ b/vl.c @@ -3757,21 +3757,18 @@ int main(int argc, char **argv, char **envp) qdev_prop_register_global(&kvm_pit_lost_tick_policy); break; } - case QEMU_OPTION_accel: { - QemuOpts *accel_opts; - + case QEMU_OPTION_accel: accel_opts = qemu_opts_parse_noisily(qemu_find_opts("accel"), optarg, true); optarg = qemu_opt_get(accel_opts, "accel"); if (!optarg || is_help_option(optarg)) { error_printf("Possible accelerators: kvm, xen, hax, tcg\n"); - exit(1); + exit(0); } - accel_opts = qemu_opts_create(qemu_find_opts("machine"), NULL, - false, &error_abort); - qemu_opt_set(accel_opts, "accel", optarg, &error_abort); + opts = qemu_opts_create(qemu_find_opts("machine"), NULL, + false, &error_abort); + qemu_opt_set(opts, "accel", optarg, &error_abort); break; - } case QEMU_OPTION_usb: olist = qemu_find_opts("machine"); qemu_opts_parse_noisily(olist, "usb=on", false);
Commit bde4d9205 ("Fix the -accel parameter and the documentation for 'hax'") introduced a regression by adding a new local accel_opts variable which shadows the variable with the same name that is declared at the beginning of the main() scope. This causes the qemu_tcg_configure() call later to be always called with NULL, so that the thread=xxx option gets ignored. Fix it by removing the local accel_opts variable and use "opts" instead, which is meant for storing temporary QemuOpts values. And while we're at it, also change the exit(1) here to exit(0) since asking for help is not an error. Fixes: bde4d9205ee9def98852ff6054cdef4efd74e1f8 Reported-by: Markus Armbruster <armbru@redhat.com> Reported-by: Emilio G. Cota <cota@braap.org> Signed-off-by: Thomas Huth <thuth@redhat.com> --- vl.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)