Thomas Huth <th...@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 <arm...@redhat.com> > Reported-by: Emilio G. Cota <c...@braap.org> > Signed-off-by: Thomas Huth <th...@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 <arm...@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 '!'.