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

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

[Thomas Huth]

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

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

[Markus Armbruster]

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

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

[Alex Bennée]

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

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

[Paolo Bonzini]

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

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

[Thomas Huth]

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