Re: [Qemu-devel] [PATCH] vl: Fix broken thread=xxx option of the --accel parameter

[Markus Armbruster]

Thomas Huth <address@hidden> 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 <address@hidden>
> Reported-by: Emilio G. Cota <address@hidden>
> Signed-off-by: Thomas Huth <address@hidden>
> ---
>  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 <address@hidden>

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 '!'.