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);
--
1.8.3.1
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 '!'.
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
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
© 2016 - 2025 Red Hat, Inc.