[PATCH 0/4] vl: Fixes for cleanups to -accel

[PATCH 0/4] vl: Fixes for cleanups to -accel

[Richard Henderson]

Running qemu-system-foo with no options should not generate
a warning for "invalid accelerator bar".

Also, fix some mistakes made while moving the code from accel/accel.c.


r~


Richard Henderson (4):
  vl: Remove unused variable in configure_accelerators
  vl: Free accel_list in configure_accelerators
  vl: Remove useless test in configure_accelerators
  vl: Only choose enabled accelerators in configure_accelerators

 vl.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

-- 
2.20.1

[PATCH 1/4] vl: Remove unused variable in configure_accelerators

[Richard Henderson]

The accel_initialised variable no longer has any setters.

Fixes: 6f6e1698a68c
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 vl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index 86474a55c9..be79b03c1a 100644
--- a/vl.c
+++ b/vl.c
@@ -2749,7 +2749,6 @@ static void configure_accelerators(const char *progname)
 {
     const char *accel;
     char **accel_list, **tmp;
-    bool accel_initialised = false;
     bool init_failed = false;
 
     qemu_opts_foreach(qemu_find_opts("icount"),
@@ -2776,7 +2775,7 @@ static void configure_accelerators(const char *progname)
 
         accel_list = g_strsplit(accel, ":", 0);
 
-        for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
+        for (tmp = accel_list; tmp && *tmp; tmp++) {
             /*
              * Filter invalid accelerators here, to prevent obscenities
              * such as "-machine accel=tcg,,thread=single".
-- 
2.20.1

[PATCH 2/4] vl: Free accel_list in configure_accelerators

[Richard Henderson]

We allocate the list with g_strsplit, so free it too.
This freeing was lost during one of the rearrangements.

Fixes: 6f6e1698a68c
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 vl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index be79b03c1a..c9329fe699 100644
--- a/vl.c
+++ b/vl.c
@@ -2748,7 +2748,6 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
 static void configure_accelerators(const char *progname)
 {
     const char *accel;
-    char **accel_list, **tmp;
     bool init_failed = false;
 
     qemu_opts_foreach(qemu_find_opts("icount"),
@@ -2756,6 +2755,8 @@ static void configure_accelerators(const char *progname)
 
     accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
     if (QTAILQ_EMPTY(&qemu_accel_opts.head)) {
+        char **accel_list, **tmp;
+
         if (accel == NULL) {
             /* Select the default accelerator */
             if (!accel_find("tcg") && !accel_find("kvm")) {
@@ -2787,6 +2788,7 @@ static void configure_accelerators(const char *progname)
                 error_report("invalid accelerator %s", *tmp);
             }
         }
+        g_strfreev(accel_list);
     } else {
         if (accel != NULL) {
             error_report("The -accel and \"-machine accel=\" options are incompatible");
-- 
2.20.1

[PATCH 3/4] vl: Remove useless test in configure_accelerators

[Richard Henderson]

The result of g_strsplit is never NULL.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 vl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index c9329fe699..887dbfbb5d 100644
--- a/vl.c
+++ b/vl.c
@@ -2776,7 +2776,7 @@ static void configure_accelerators(const char *progname)
 
         accel_list = g_strsplit(accel, ":", 0);
 
-        for (tmp = accel_list; tmp && *tmp; tmp++) {
+        for (tmp = accel_list; *tmp; tmp++) {
             /*
              * Filter invalid accelerators here, to prevent obscenities
              * such as "-machine accel=tcg,,thread=single".
-- 
2.20.1

[PATCH 4/4] vl: Only choose enabled accelerators in configure_accelerators

[Richard Henderson]

By choosing "tcg:kvm" when kvm is not enabled, we generate
an incorrect warning: "invalid accelerator kvm".

Presumably the inverse is also true with --disable-tcg.

Fixes: 28a0961757fc
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 vl.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/vl.c b/vl.c
index 887dbfbb5d..9b7651c80d 100644
--- a/vl.c
+++ b/vl.c
@@ -2759,11 +2759,10 @@ static void configure_accelerators(const char *progname)
 
         if (accel == NULL) {
             /* Select the default accelerator */
-            if (!accel_find("tcg") && !accel_find("kvm")) {
-                error_report("No accelerator selected and"
-                             " no default accelerator available");
-                exit(1);
-            } else {
+            bool have_tcg = accel_find("tcg");
+            bool have_kvm = accel_find("kvm");
+
+            if (have_tcg && have_kvm) {
                 int pnlen = strlen(progname);
                 if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
                     /* If the program name ends with "kvm", we prefer KVM */
@@ -2771,9 +2770,16 @@ static void configure_accelerators(const char *progname)
                 } else {
                     accel = "tcg:kvm";
                 }
+            } else if (have_kvm) {
+                accel = "kvm";
+            } else if (have_tcg) {
+                accel = "tcg";
+            } else {
+                error_report("No accelerator selected and"
+                             " no default accelerator available");
+                exit(1);
             }
         }
-
         accel_list = g_strsplit(accel, ":", 0);
 
         for (tmp = accel_list; *tmp; tmp++) {
-- 
2.20.1

Re: [PATCH 2/4] vl: Free accel_list in configure_accelerators

[Thomas Huth]

On 09/01/2020 03.17, Richard Henderson wrote:
> We allocate the list with g_strsplit, so free it too.
> This freeing was lost during one of the rearrangements.
> 
> Fixes: 6f6e1698a68c
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  vl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index be79b03c1a..c9329fe699 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2748,7 +2748,6 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>  static void configure_accelerators(const char *progname)
>  {
>      const char *accel;
> -    char **accel_list, **tmp;
>      bool init_failed = false;
>  
>      qemu_opts_foreach(qemu_find_opts("icount"),
> @@ -2756,6 +2755,8 @@ static void configure_accelerators(const char *progname)
>  
>      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
>      if (QTAILQ_EMPTY(&qemu_accel_opts.head)) {
> +        char **accel_list, **tmp;
> +
>          if (accel == NULL) {
>              /* Select the default accelerator */
>              if (!accel_find("tcg") && !accel_find("kvm")) {
> @@ -2787,6 +2788,7 @@ static void configure_accelerators(const char *progname)
>                  error_report("invalid accelerator %s", *tmp);
>              }
>          }
> +        g_strfreev(accel_list);
>      } else {
>          if (accel != NULL) {
>              error_report("The -accel and \"-machine accel=\" options are incompatible");
> 

FYI, a fix for this is already part of Laurent's "Trivial branch
patches" PULL request from yesterday.

 Thomas

Re: [PATCH 2/4] vl: Free accel_list in configure_accelerators

[Laurent Vivier]

On 09/01/2020 09:18, Thomas Huth wrote:
> On 09/01/2020 03.17, Richard Henderson wrote:
>> We allocate the list with g_strsplit, so free it too.
>> This freeing was lost during one of the rearrangements.
>>
>> Fixes: 6f6e1698a68c
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  vl.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/vl.c b/vl.c
>> index be79b03c1a..c9329fe699 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2748,7 +2748,6 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>  static void configure_accelerators(const char *progname)
>>  {
>>      const char *accel;
>> -    char **accel_list, **tmp;
>>      bool init_failed = false;
>>  
>>      qemu_opts_foreach(qemu_find_opts("icount"),
>> @@ -2756,6 +2755,8 @@ static void configure_accelerators(const char *progname)
>>  
>>      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
>>      if (QTAILQ_EMPTY(&qemu_accel_opts.head)) {
>> +        char **accel_list, **tmp;
>> +
>>          if (accel == NULL) {
>>              /* Select the default accelerator */
>>              if (!accel_find("tcg") && !accel_find("kvm")) {
>> @@ -2787,6 +2788,7 @@ static void configure_accelerators(const char *progname)
>>                  error_report("invalid accelerator %s", *tmp);
>>              }
>>          }
>> +        g_strfreev(accel_list);
>>      } else {
>>          if (accel != NULL) {
>>              error_report("The -accel and \"-machine accel=\" options are incompatible");
>>
> 
> FYI, a fix for this is already part of Laurent's "Trivial branch
> patches" PULL request from yesterday.

https://patchew.org/QEMU/20200108160233.991134-1-laurent@vivier.eu/20200108160233.991134-6-laurent@vivier.eu/

Thanks,
Laurent

Re: [PATCH 0/4] vl: Fixes for cleanups to -accel

[Paolo Bonzini]

On 09/01/20 03:17, Richard Henderson wrote:
> Running qemu-system-foo with no options should not generate
> a warning for "invalid accelerator bar".
> 
> Also, fix some mistakes made while moving the code from accel/accel.c.
> 
> 
> r~
> 
> 
> Richard Henderson (4):
>   vl: Remove unused variable in configure_accelerators
>   vl: Free accel_list in configure_accelerators
>   vl: Remove useless test in configure_accelerators
>   vl: Only choose enabled accelerators in configure_accelerators
> 
>  vl.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

I've queued it, but feel free to beat me to sending a pull request.

Paolo

Re: [PATCH 1/4] vl: Remove unused variable in configure_accelerators

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> The accel_initialised variable no longer has any setters.
>
> Fixes: 6f6e1698a68c
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  vl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 86474a55c9..be79b03c1a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2749,7 +2749,6 @@ static void configure_accelerators(const char *progname)
>  {
>      const char *accel;
>      char **accel_list, **tmp;
> -    bool accel_initialised = false;
>      bool init_failed = false;
>  
>      qemu_opts_foreach(qemu_find_opts("icount"),
> @@ -2776,7 +2775,7 @@ static void configure_accelerators(const char *progname)
>  
>          accel_list = g_strsplit(accel, ":", 0);
>  
> -        for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
> +        for (tmp = accel_list; tmp && *tmp; tmp++) {
>              /*
>               * Filter invalid accelerators here, to prevent obscenities
>               * such as "-machine accel=tcg,,thread=single".


-- 
Alex Bennée

Re: [PATCH 2/4] vl: Free accel_list in configure_accelerators

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> We allocate the list with g_strsplit, so free it too.
> This freeing was lost during one of the rearrangements.
>
> Fixes: 6f6e1698a68c
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  vl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/vl.c b/vl.c
> index be79b03c1a..c9329fe699 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2748,7 +2748,6 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>  static void configure_accelerators(const char *progname)
>  {
>      const char *accel;
> -    char **accel_list, **tmp;
>      bool init_failed = false;
>  
>      qemu_opts_foreach(qemu_find_opts("icount"),
> @@ -2756,6 +2755,8 @@ static void configure_accelerators(const char *progname)
>  
>      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
>      if (QTAILQ_EMPTY(&qemu_accel_opts.head)) {
> +        char **accel_list, **tmp;
> +
>          if (accel == NULL) {
>              /* Select the default accelerator */
>              if (!accel_find("tcg") && !accel_find("kvm")) {
> @@ -2787,6 +2788,7 @@ static void configure_accelerators(const char *progname)
>                  error_report("invalid accelerator %s", *tmp);
>              }
>          }
> +        g_strfreev(accel_list);
>      } else {
>          if (accel != NULL) {
>              error_report("The -accel and \"-machine accel=\" options are incompatible");


-- 
Alex Bennée

Re: [PATCH 3/4] vl: Remove useless test in configure_accelerators

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> The result of g_strsplit is never NULL.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  vl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/vl.c b/vl.c
> index c9329fe699..887dbfbb5d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2776,7 +2776,7 @@ static void configure_accelerators(const char *progname)
>  
>          accel_list = g_strsplit(accel, ":", 0);
>  
> -        for (tmp = accel_list; tmp && *tmp; tmp++) {
> +        for (tmp = accel_list; *tmp; tmp++) {
>              /*
>               * Filter invalid accelerators here, to prevent obscenities
>               * such as "-machine accel=tcg,,thread=single".


-- 
Alex Bennée

Re: [PATCH 4/4] vl: Only choose enabled accelerators in configure_accelerators

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> By choosing "tcg:kvm" when kvm is not enabled, we generate
> an incorrect warning: "invalid accelerator kvm".
>
> Presumably the inverse is also true with --disable-tcg.
>
> Fixes: 28a0961757fc
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  vl.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 887dbfbb5d..9b7651c80d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2759,11 +2759,10 @@ static void configure_accelerators(const char *progname)
>  
>          if (accel == NULL) {
>              /* Select the default accelerator */
> -            if (!accel_find("tcg") && !accel_find("kvm")) {
> -                error_report("No accelerator selected and"
> -                             " no default accelerator available");
> -                exit(1);
> -            } else {
> +            bool have_tcg = accel_find("tcg");
> +            bool have_kvm = accel_find("kvm");
> +
> +            if (have_tcg && have_kvm) {
>                  int pnlen = strlen(progname);
>                  if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3],
>          "kvm")) {

I know you're not touching this bit but:

modified   vl.c
@@ -2763,8 +2763,7 @@ static void configure_accelerators(const char *progname)
             bool have_kvm = accel_find("kvm");
 
             if (have_tcg && have_kvm) {
-                int pnlen = strlen(progname);
-                if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
+                if (g_str_has_suffix(progname, "kvm")) {
                     /* If the program name ends with "kvm", we prefer KVM */
                     accel = "kvm:tcg";
                 } else {


>                      /* If the program name ends with "kvm", we prefer KVM */
> @@ -2771,9 +2770,16 @@ static void configure_accelerators(const char *progname)
>                  } else {
>                      accel = "tcg:kvm";
>                  }
> +            } else if (have_kvm) {
> +                accel = "kvm";
> +            } else if (have_tcg) {
> +                accel = "tcg";
> +            } else {
> +                error_report("No accelerator selected and"
> +                             " no default accelerator available");
> +                exit(1);
>              }
>          }
> -
>          accel_list = g_strsplit(accel, ":", 0);
>  
>          for (tmp = accel_list; *tmp; tmp++) {

Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

[PATCH 2/4] vl: Free accel_list in configure_accelerators

[Aleksandar Markovic]

[-- Attachment #1: Type: text/plain, Size: 2224 bytes --]

On Thursday, January 9, 2020, Laurent Vivier <lvivier@redhat.com> wrote:

> On 09/01/2020 09:18, Thomas Huth wrote:
> > On 09/01/2020 03.17, Richard Henderson wrote:
> >> We allocate the list with g_strsplit, so free it too.
> >> This freeing was lost during one of the rearrangements.
> >>
> >> Fixes: 6f6e1698a68c
> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> >> ---
> >>  vl.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/vl.c b/vl.c
> >> index be79b03c1a..c9329fe699 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -2748,7 +2748,6 @@ static int do_configure_accelerator(void *opaque,
> QemuOpts *opts, Error **errp)
> >>  static void configure_accelerators(const char *progname)
> >>  {
> >>      const char *accel;
> >> -    char **accel_list, **tmp;
> >>      bool init_failed = false;
> >>
> >>      qemu_opts_foreach(qemu_find_opts("icount"),
> >> @@ -2756,6 +2755,8 @@ static void configure_accelerators(const char
> *progname)
> >>
> >>      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
> >>      if (QTAILQ_EMPTY(&qemu_accel_opts.head)) {
> >> +        char **accel_list, **tmp;
> >> +
> >>          if (accel == NULL) {
> >>              /* Select the default accelerator */
> >>              if (!accel_find("tcg") && !accel_find("kvm")) {
> >> @@ -2787,6 +2788,7 @@ static void configure_accelerators(const char
> *progname)
> >>                  error_report("invalid accelerator %s", *tmp);
> >>              }
> >>          }
> >> +        g_strfreev(accel_list);
> >>      } else {
> >>          if (accel != NULL) {
> >>              error_report("The -accel and \"-machine accel=\" options
> are incompatible");
> >>
> >
> > FYI, a fix for this is already part of Laurent's "Trivial branch
> > patches" PULL request from yesterday.
>
> https://patchew.org/QEMU/20200108160233.991134-1-laurent@
> vivier.eu/20200108160233.991134-6-laurent@vivier.eu/
>
>
If this (from Laurent's PR) patch is merged, Richard could transform his
patch into "vl: Fix the scope of variables accel_list and tmp in
configure_accelerator()" in v2.

Whatever happens:

Reviewed by: Aleksandar Markovic <amarkovic@wavecomp.com>


> Thanks,
> Laurent
>
>
>

[-- Attachment #2: Type: text/html, Size: 3493 bytes --]

Re: [PATCH 1/4] vl: Remove unused variable in configure_accelerators

[Aleksandar Markovic]

[-- Attachment #1: Type: text/plain, Size: 1185 bytes --]

On Thursday, January 9, 2020, Richard Henderson <
richard.henderson@linaro.org> wrote:

> The accel_initialised variable no longer has any setters.
>
> Fixes: 6f6e1698a68c
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  vl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
>
Reviewed by: Aleksandar Markovic <amarkovic@wavecomp.com>


> diff --git a/vl.c b/vl.c
> index 86474a55c9..be79b03c1a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2749,7 +2749,6 @@ static void configure_accelerators(const char
> *progname)
>  {
>      const char *accel;
>      char **accel_list, **tmp;
> -    bool accel_initialised = false;
>      bool init_failed = false;
>
>      qemu_opts_foreach(qemu_find_opts("icount"),
> @@ -2776,7 +2775,7 @@ static void configure_accelerators(const char
> *progname)
>
>          accel_list = g_strsplit(accel, ":", 0);
>
> -        for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
> +        for (tmp = accel_list; tmp && *tmp; tmp++) {
>              /*
>               * Filter invalid accelerators here, to prevent obscenities
>               * such as "-machine accel=tcg,,thread=single".
> --
> 2.20.1
>
>
>

[-- Attachment #2: Type: text/html, Size: 2081 bytes --]

Re: [PATCH 3/4] vl: Remove useless test in configure_accelerators

[Aleksandar Markovic]

[-- Attachment #1: Type: text/plain, Size: 843 bytes --]

On Thursday, January 9, 2020, Richard Henderson <
richard.henderson@linaro.org> wrote:

> The result of g_strsplit is never NULL.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  vl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
>
Reviewed by: Aleksandar Markovic <amarkovic@wavecomp.com>



> diff --git a/vl.c b/vl.c
> index c9329fe699..887dbfbb5d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2776,7 +2776,7 @@ static void configure_accelerators(const char
> *progname)
>
>          accel_list = g_strsplit(accel, ":", 0);
>
> -        for (tmp = accel_list; tmp && *tmp; tmp++) {
> +        for (tmp = accel_list; *tmp; tmp++) {
>              /*
>               * Filter invalid accelerators here, to prevent obscenities
>               * such as "-machine accel=tcg,,thread=single".
> --
> 2.20.1
>
>
>

[-- Attachment #2: Type: text/html, Size: 1681 bytes --]

[PATCH 4/4] vl: Only choose enabled accelerators in configure_accelerators

[Aleksandar Markovic]

[-- Attachment #1: Type: text/plain, Size: 3086 bytes --]

On Thursday, January 9, 2020, Alex Bennée <alex.bennee@linaro.org> wrote:

>
> Richard Henderson <richard.henderson@linaro.org> writes:
>
> > By choosing "tcg:kvm" when kvm is not enabled, we generate
> > an incorrect warning: "invalid accelerator kvm".
> >
> > Presumably the inverse is also true with --disable-tcg.
> >
> > Fixes: 28a0961757fc
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >  vl.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/vl.c b/vl.c
> > index 887dbfbb5d..9b7651c80d 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2759,11 +2759,10 @@ static void configure_accelerators(const char
> *progname)
> >
> >          if (accel == NULL) {
> >              /* Select the default accelerator */
> > -            if (!accel_find("tcg") && !accel_find("kvm")) {
> > -                error_report("No accelerator selected and"
> > -                             " no default accelerator available");
> > -                exit(1);
> > -            } else {
> > +            bool have_tcg = accel_find("tcg");
> > +            bool have_kvm = accel_find("kvm");
> > +
> > +            if (have_tcg && have_kvm) {
> >                  int pnlen = strlen(progname);
> >                  if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3],
> >          "kvm")) {
>
> I know you're not touching this bit but:
>
> modified   vl.c
> @@ -2763,8 +2763,7 @@ static void configure_accelerators(const char
> *progname)
>              bool have_kvm = accel_find("kvm");
>
>              if (have_tcg && have_kvm) {
> -                int pnlen = strlen(progname);
> -                if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3],
> "kvm")) {
> +                if (g_str_has_suffix(progname, "kvm")) {
>                      /* If the program name ends with "kvm", we prefer KVM
> */
>                      accel = "kvm:tcg";
>                  } else {
>
>
> >                      /* If the program name ends with "kvm", we prefer
> KVM */
> > @@ -2771,9 +2770,16 @@ static void configure_accelerators(const char
> *progname)
> >                  } else {
> >                      accel = "tcg:kvm";
> >                  }
> > +            } else if (have_kvm) {
> > +                accel = "kvm";
> > +            } else if (have_tcg) {
> > +                accel = "tcg";
> > +            } else {
> > +                error_report("No accelerator selected and"
> > +                             " no default accelerator available");
> > +                exit(1);
> >              }
> >          }
> > -
> >          accel_list = g_strsplit(accel, ":", 0);
> >
> >          for (tmp = accel_list; *tmp; tmp++) {
>
> Anyway:
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>
>
With or without Alex' hint:

Reviewed by: Aleksandar Markovic <amarkovic@wavecomp.com>

Alex' hint could be a separate patch. If you decide to do it, for that
patch too you can include my r-b.


-- 
> Alex Bennée
>
>

[-- Attachment #2: Type: text/html, Size: 4862 bytes --]

Re: [PATCH 1/4] vl: Remove unused variable in configure_accelerators

[Philippe Mathieu-Daudé]

On 1/9/20 3:17 AM, Richard Henderson wrote:
> The accel_initialised variable no longer has any setters.
> 
> Fixes: 6f6e1698a68c
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   vl.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 86474a55c9..be79b03c1a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2749,7 +2749,6 @@ static void configure_accelerators(const char *progname)
>   {
>       const char *accel;
>       char **accel_list, **tmp;
> -    bool accel_initialised = false;
>       bool init_failed = false;
>   
>       qemu_opts_foreach(qemu_find_opts("icount"),
> @@ -2776,7 +2775,7 @@ static void configure_accelerators(const char *progname)
>   
>           accel_list = g_strsplit(accel, ":", 0);
>   
> -        for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
> +        for (tmp = accel_list; tmp && *tmp; tmp++) {
>               /*
>                * Filter invalid accelerators here, to prevent obscenities
>                * such as "-machine accel=tcg,,thread=single".
> 

Re: [PATCH 3/4] vl: Remove useless test in configure_accelerators

[Philippe Mathieu-Daudé]

On 1/9/20 3:17 AM, Richard Henderson wrote:
> The result of g_strsplit is never NULL.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   vl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index c9329fe699..887dbfbb5d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2776,7 +2776,7 @@ static void configure_accelerators(const char *progname)
>   
>           accel_list = g_strsplit(accel, ":", 0);
>   
> -        for (tmp = accel_list; tmp && *tmp; tmp++) {
> +        for (tmp = accel_list; *tmp; tmp++) {
>               /*
>                * Filter invalid accelerators here, to prevent obscenities
>                * such as "-machine accel=tcg,,thread=single".
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>