[Qemu-devel] [PATCH for-2.0 V4] tests/acpi-test: do not fail if iasl is broken

[Qemu-devel] [PATCH for-2.0 V4] tests/acpi-test: do not fail if iasl is broken

[Marcel Apfelbaum]

There is an issue with iasl on big endian machines: It
cannot disassemble acpi tables taken from little endian
machines, so we cannot check the expected tables.

The acpi test will check if the expected aml files
can be disassembled, and will issue an warning not
failing the test on those machines until this
problem is solved by the acpica community.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
V3 -> V4:
 Addressed all upstream comments:
 - Instead of disabling iasl for big endian machines,
   the test checks if the expected aml files can be
   disassembled, if not it issues a warning instead
   of failing the test

V2 -> V3:
 Addressed Michael S. Tsirkin's review:
 - tests don't need to re-run detection, use configure
   to figure out if it is an LE machine.

V1 -> V2:
 Addressed an offline tip for a much cleaner
 macro line, thanks!

 tests/acpi-test.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/tests/acpi-test.c b/tests/acpi-test.c
index 249fe03..76fbccf 100644
--- a/tests/acpi-test.c
+++ b/tests/acpi-test.c
@@ -456,13 +456,12 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
     /* pass 'out' and 'out_err' in order to be redirected */
     ret = g_spawn_command_line_sync(command_line->str, &out, &out_err, NULL, &error);
     g_assert_no_error(error);
-
     if (ret) {
         ret = g_file_get_contents(sdt->asl_file, (gchar **)&sdt->asl,
                                   &sdt->asl_len, &error);
         g_assert(ret);
         g_assert_no_error(error);
-        g_assert(sdt->asl_len);
+        ret = (sdt->asl_len > 0);
     }
 
     g_free(out);
@@ -560,15 +559,20 @@ static void test_acpi_asl(test_data *data)
         g_assert(!err || exp_err);
 
         if (g_strcmp0(asl->str, exp_asl->str)) {
-            uint32_t signature = cpu_to_le32(exp_sdt->header.signature);
-            sdt->tmp_files_retain = true;
-            exp_sdt->tmp_files_retain = true;
-            fprintf(stderr,
-                    "acpi-test: Warning! %.4s mismatch. "
-                    "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n",
-                    (gchar *)&signature,
-                    sdt->asl_file, sdt->aml_file,
-                    exp_sdt->asl_file, exp_sdt->aml_file);
+            if (exp_err) {
+                fprintf(stderr,
+                        "Warning! iasl couldn't parse the expected aml\n");
+            } else {
+                uint32_t signature = cpu_to_le32(exp_sdt->header.signature);
+                sdt->tmp_files_retain = true;
+                exp_sdt->tmp_files_retain = true;
+                fprintf(stderr,
+                        "acpi-test: Warning! %.4s mismatch. "
+                        "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n",
+                        (gchar *)&signature,
+                        sdt->asl_file, sdt->aml_file,
+                        exp_sdt->asl_file, exp_sdt->aml_file);
+          }
         }
         g_string_free(asl, true);
         g_string_free(exp_asl, true);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.0 V4] tests/acpi-test: do not fail if iasl is broken

[Andreas Färber]

Am 24.03.2014 11:02, schrieb Marcel Apfelbaum:
> There is an issue with iasl on big endian machines: It
> cannot disassemble acpi tables taken from little endian
> machines, so we cannot check the expected tables.
> 
> The acpi test will check if the expected aml files
> can be disassembled, and will issue an warning not
> failing the test on those machines until this
> problem is solved by the acpica community.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
> V3 -> V4:
>  Addressed all upstream comments:
>  - Instead of disabling iasl for big endian machines,
>    the test checks if the expected aml files can be
>    disassembled, if not it issues a warning instead
>    of failing the test
> 
> V2 -> V3:
>  Addressed Michael S. Tsirkin's review:
>  - tests don't need to re-run detection, use configure
>    to figure out if it is an LE machine.
> 
> V1 -> V2:
>  Addressed an offline tip for a much cleaner
>  macro line, thanks!
> 
>  tests/acpi-test.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/acpi-test.c b/tests/acpi-test.c
> index 249fe03..76fbccf 100644
> --- a/tests/acpi-test.c
> +++ b/tests/acpi-test.c
> @@ -456,13 +456,12 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
>      /* pass 'out' and 'out_err' in order to be redirected */
>      ret = g_spawn_command_line_sync(command_line->str, &out, &out_err, NULL, &error);
>      g_assert_no_error(error);
> -
>      if (ret) {
>          ret = g_file_get_contents(sdt->asl_file, (gchar **)&sdt->asl,
>                                    &sdt->asl_len, &error);
>          g_assert(ret);
>          g_assert_no_error(error);
> -        g_assert(sdt->asl_len);
> +        ret = (sdt->asl_len > 0);
>      }
>  
>      g_free(out);
> @@ -560,15 +559,20 @@ static void test_acpi_asl(test_data *data)
>          g_assert(!err || exp_err);
>  
>          if (g_strcmp0(asl->str, exp_asl->str)) {
> -            uint32_t signature = cpu_to_le32(exp_sdt->header.signature);
> -            sdt->tmp_files_retain = true;
> -            exp_sdt->tmp_files_retain = true;
> -            fprintf(stderr,
> -                    "acpi-test: Warning! %.4s mismatch. "
> -                    "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n",
> -                    (gchar *)&signature,
> -                    sdt->asl_file, sdt->aml_file,
> -                    exp_sdt->asl_file, exp_sdt->aml_file);
> +            if (exp_err) {
> +                fprintf(stderr,
> +                        "Warning! iasl couldn't parse the expected aml\n");

Should these two be g_test_message()s? That's used in other tests -
downside is the message appears in the .xml files generated as
intermediary for the HTML report but did (in my case) not show up on
stdout/stderr. Maybe we should try to fix that? Paolo? Stefan?

Regards,
Andreas

> +            } else {
> +                uint32_t signature = cpu_to_le32(exp_sdt->header.signature);
> +                sdt->tmp_files_retain = true;
> +                exp_sdt->tmp_files_retain = true;
> +                fprintf(stderr,
> +                        "acpi-test: Warning! %.4s mismatch. "
> +                        "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n",
> +                        (gchar *)&signature,
> +                        sdt->asl_file, sdt->aml_file,
> +                        exp_sdt->asl_file, exp_sdt->aml_file);
> +          }
>          }
>          g_string_free(asl, true);
>          g_string_free(exp_asl, true);
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-2.0 V4] tests/acpi-test: do not fail if iasl is broken

[Michael S. Tsirkin]

On Mon, Mar 24, 2014 at 12:02:33PM +0200, Marcel Apfelbaum wrote:
> There is an issue with iasl on big endian machines: It
> cannot disassemble acpi tables taken from little endian
> machines, so we cannot check the expected tables.
> 
> The acpi test will check if the expected aml files
> can be disassembled, and will issue an warning not
> failing the test on those machines until this
> problem is solved by the acpica community.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

Applied, improvements such as these suggested by Andreas
can be incremental on top.

> ---
> V3 -> V4:
>  Addressed all upstream comments:
>  - Instead of disabling iasl for big endian machines,
>    the test checks if the expected aml files can be
>    disassembled, if not it issues a warning instead
>    of failing the test
> 
> V2 -> V3:
>  Addressed Michael S. Tsirkin's review:
>  - tests don't need to re-run detection, use configure
>    to figure out if it is an LE machine.
> 
> V1 -> V2:
>  Addressed an offline tip for a much cleaner
>  macro line, thanks!
> 
>  tests/acpi-test.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/acpi-test.c b/tests/acpi-test.c
> index 249fe03..76fbccf 100644
> --- a/tests/acpi-test.c
> +++ b/tests/acpi-test.c
> @@ -456,13 +456,12 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
>      /* pass 'out' and 'out_err' in order to be redirected */
>      ret = g_spawn_command_line_sync(command_line->str, &out, &out_err, NULL, &error);
>      g_assert_no_error(error);
> -
>      if (ret) {
>          ret = g_file_get_contents(sdt->asl_file, (gchar **)&sdt->asl,
>                                    &sdt->asl_len, &error);
>          g_assert(ret);
>          g_assert_no_error(error);
> -        g_assert(sdt->asl_len);
> +        ret = (sdt->asl_len > 0);
>      }
>  
>      g_free(out);
> @@ -560,15 +559,20 @@ static void test_acpi_asl(test_data *data)
>          g_assert(!err || exp_err);
>  
>          if (g_strcmp0(asl->str, exp_asl->str)) {
> -            uint32_t signature = cpu_to_le32(exp_sdt->header.signature);
> -            sdt->tmp_files_retain = true;
> -            exp_sdt->tmp_files_retain = true;
> -            fprintf(stderr,
> -                    "acpi-test: Warning! %.4s mismatch. "
> -                    "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n",
> -                    (gchar *)&signature,
> -                    sdt->asl_file, sdt->aml_file,
> -                    exp_sdt->asl_file, exp_sdt->aml_file);
> +            if (exp_err) {
> +                fprintf(stderr,
> +                        "Warning! iasl couldn't parse the expected aml\n");
> +            } else {
> +                uint32_t signature = cpu_to_le32(exp_sdt->header.signature);
> +                sdt->tmp_files_retain = true;
> +                exp_sdt->tmp_files_retain = true;
> +                fprintf(stderr,
> +                        "acpi-test: Warning! %.4s mismatch. "
> +                        "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n",
> +                        (gchar *)&signature,
> +                        sdt->asl_file, sdt->aml_file,
> +                        exp_sdt->asl_file, exp_sdt->aml_file);
> +          }
>          }
>          g_string_free(asl, true);
>          g_string_free(exp_asl, true);
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH for-2.0 V4] tests/acpi-test: do not fail if iasl is broken

[Marcel Apfelbaum]

On Mon, 2014-03-24 at 11:33 +0100, Andreas Färber wrote:
> Am 24.03.2014 11:02, schrieb Marcel Apfelbaum:
> > There is an issue with iasl on big endian machines: It
> > cannot disassemble acpi tables taken from little endian
> > machines, so we cannot check the expected tables.
> > 
> > The acpi test will check if the expected aml files
> > can be disassembled, and will issue an warning not
> > failing the test on those machines until this
> > problem is solved by the acpica community.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> > V3 -> V4:
> >  Addressed all upstream comments:
> >  - Instead of disabling iasl for big endian machines,
> >    the test checks if the expected aml files can be
> >    disassembled, if not it issues a warning instead
> >    of failing the test
> > 
> > V2 -> V3:
> >  Addressed Michael S. Tsirkin's review:
> >  - tests don't need to re-run detection, use configure
> >    to figure out if it is an LE machine.
> > 
> > V1 -> V2:
> >  Addressed an offline tip for a much cleaner
> >  macro line, thanks!
> > 
> >  tests/acpi-test.c | 26 +++++++++++++++-----------
> >  1 file changed, 15 insertions(+), 11 deletions(-)
> > 
> > diff --git a/tests/acpi-test.c b/tests/acpi-test.c
> > index 249fe03..76fbccf 100644
> > --- a/tests/acpi-test.c
> > +++ b/tests/acpi-test.c
> > @@ -456,13 +456,12 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
> >      /* pass 'out' and 'out_err' in order to be redirected */
> >      ret = g_spawn_command_line_sync(command_line->str, &out, &out_err, NULL, &error);
> >      g_assert_no_error(error);
> > -
> >      if (ret) {
> >          ret = g_file_get_contents(sdt->asl_file, (gchar **)&sdt->asl,
> >                                    &sdt->asl_len, &error);
> >          g_assert(ret);
> >          g_assert_no_error(error);
> > -        g_assert(sdt->asl_len);
> > +        ret = (sdt->asl_len > 0);
> >      }
> >  
> >      g_free(out);
> > @@ -560,15 +559,20 @@ static void test_acpi_asl(test_data *data)
> >          g_assert(!err || exp_err);
> >  
> >          if (g_strcmp0(asl->str, exp_asl->str)) {
> > -            uint32_t signature = cpu_to_le32(exp_sdt->header.signature);
> > -            sdt->tmp_files_retain = true;
> > -            exp_sdt->tmp_files_retain = true;
> > -            fprintf(stderr,
> > -                    "acpi-test: Warning! %.4s mismatch. "
> > -                    "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n",
> > -                    (gchar *)&signature,
> > -                    sdt->asl_file, sdt->aml_file,
> > -                    exp_sdt->asl_file, exp_sdt->aml_file);
> > +            if (exp_err) {
> > +                fprintf(stderr,
> > +                        "Warning! iasl couldn't parse the expected aml\n");
> 
> Should these two be g_test_message()s? That's used in other tests -
> downside is the message appears in the .xml files generated as
> intermediary for the HTML report but did (in my case) not show up on
> stdout/stderr. Maybe we should try to fix that? Paolo? Stefan?
I also don't see them in stdout/stderr. This is the *only*
reason I use fprintf.

Thanks,
Marcel

> 
> Regards,
> Andreas
> 
> > +            } else {
> > +                uint32_t signature = cpu_to_le32(exp_sdt->header.signature);
> > +                sdt->tmp_files_retain = true;
> > +                exp_sdt->tmp_files_retain = true;
> > +                fprintf(stderr,
> > +                        "acpi-test: Warning! %.4s mismatch. "
> > +                        "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n",
> > +                        (gchar *)&signature,
> > +                        sdt->asl_file, sdt->aml_file,
> > +                        exp_sdt->asl_file, exp_sdt->aml_file);
> > +          }
> >          }
> >          g_string_free(asl, true);
> >          g_string_free(exp_asl, true);
> > 
> 
> 

Re: [Qemu-devel] [PATCH for-2.0 V4] tests/acpi-test: do not fail if iasl is broken

[Andreas Färber]

Am 24.03.2014 11:42, schrieb Michael S. Tsirkin:
> On Mon, Mar 24, 2014 at 12:02:33PM +0200, Marcel Apfelbaum wrote:
>> There is an issue with iasl on big endian machines: It
>> cannot disassemble acpi tables taken from little endian
>> machines, so we cannot check the expected tables.
>>
>> The acpi test will check if the expected aml files
>> can be disassembled, and will issue an warning not
>> failing the test on those machines until this
>> problem is solved by the acpica community.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> 
> Applied, improvements such as these suggested by Andreas
> can be incremental on top.

Agreed.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-2.0 V4] tests/acpi-test: do not fail if iasl is broken

[Stefan Hajnoczi]

On Mon, Mar 24, 2014 at 02:17:10PM +0200, Marcel Apfelbaum wrote:
> On Mon, 2014-03-24 at 11:33 +0100, Andreas Färber wrote:
> > Am 24.03.2014 11:02, schrieb Marcel Apfelbaum:
> > > There is an issue with iasl on big endian machines: It
> > > cannot disassemble acpi tables taken from little endian
> > > machines, so we cannot check the expected tables.
> > > 
> > > The acpi test will check if the expected aml files
> > > can be disassembled, and will issue an warning not
> > > failing the test on those machines until this
> > > problem is solved by the acpica community.
> > > 
> > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > ---
> > > V3 -> V4:
> > >  Addressed all upstream comments:
> > >  - Instead of disabling iasl for big endian machines,
> > >    the test checks if the expected aml files can be
> > >    disassembled, if not it issues a warning instead
> > >    of failing the test
> > > 
> > > V2 -> V3:
> > >  Addressed Michael S. Tsirkin's review:
> > >  - tests don't need to re-run detection, use configure
> > >    to figure out if it is an LE machine.
> > > 
> > > V1 -> V2:
> > >  Addressed an offline tip for a much cleaner
> > >  macro line, thanks!
> > > 
> > >  tests/acpi-test.c | 26 +++++++++++++++-----------
> > >  1 file changed, 15 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/tests/acpi-test.c b/tests/acpi-test.c
> > > index 249fe03..76fbccf 100644
> > > --- a/tests/acpi-test.c
> > > +++ b/tests/acpi-test.c
> > > @@ -456,13 +456,12 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
> > >      /* pass 'out' and 'out_err' in order to be redirected */
> > >      ret = g_spawn_command_line_sync(command_line->str, &out, &out_err, NULL, &error);
> > >      g_assert_no_error(error);
> > > -
> > >      if (ret) {
> > >          ret = g_file_get_contents(sdt->asl_file, (gchar **)&sdt->asl,
> > >                                    &sdt->asl_len, &error);
> > >          g_assert(ret);
> > >          g_assert_no_error(error);
> > > -        g_assert(sdt->asl_len);
> > > +        ret = (sdt->asl_len > 0);
> > >      }
> > >  
> > >      g_free(out);
> > > @@ -560,15 +559,20 @@ static void test_acpi_asl(test_data *data)
> > >          g_assert(!err || exp_err);
> > >  
> > >          if (g_strcmp0(asl->str, exp_asl->str)) {
> > > -            uint32_t signature = cpu_to_le32(exp_sdt->header.signature);
> > > -            sdt->tmp_files_retain = true;
> > > -            exp_sdt->tmp_files_retain = true;
> > > -            fprintf(stderr,
> > > -                    "acpi-test: Warning! %.4s mismatch. "
> > > -                    "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n",
> > > -                    (gchar *)&signature,
> > > -                    sdt->asl_file, sdt->aml_file,
> > > -                    exp_sdt->asl_file, exp_sdt->aml_file);
> > > +            if (exp_err) {
> > > +                fprintf(stderr,
> > > +                        "Warning! iasl couldn't parse the expected aml\n");
> > 
> > Should these two be g_test_message()s? That's used in other tests -
> > downside is the message appears in the .xml files generated as
> > intermediary for the HTML report but did (in my case) not show up on
> > stdout/stderr. Maybe we should try to fix that? Paolo? Stefan?
> I also don't see them in stdout/stderr. This is the *only*
> reason I use fprintf.

Did you try running gtester without -q or even with -v?  (You can edit
tests/Makefile to do that.)

Stefan

Re: [Qemu-devel] [PATCH for-2.0 V4] tests/acpi-test: do not fail if iasl is broken

[Peter Maydell]

On 24 March 2014 13:23, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> Did you try running gtester without -q or even with -v?  (You can edit
> tests/Makefile to do that.)

...or use make V=1.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.0 V4] tests/acpi-test: do not fail if iasl is broken

[Marcel Apfelbaum]

On Mon, 2014-03-24 at 13:24 +0000, Peter Maydell wrote:
> On 24 March 2014 13:23, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > Did you try running gtester without -q or even with -v?  (You can edit
> > tests/Makefile to do that.)
> 
> ...or use make V=1.
I'll try both, but do we really want to run make check with extra flags
in order to see g_test_message()s ?

Thanks,
Marcel

> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH for-2.0 V4] tests/acpi-test: do not fail if iasl is broken

[Marcel Apfelbaum]

On Mon, 2014-03-24 at 13:24 +0000, Peter Maydell wrote:
> On 24 March 2014 13:23, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > Did you try running gtester without -q or even with -v?  (You can edit
> > tests/Makefile to do that.)
> 
> ...or use make V=1.
I tried the verbose mode, no g_test_message output.

Thanks,
Marcel

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH for-2.0 V4] tests/acpi-test: do not fail if iasl is broken

[Marcel Apfelbaum]

On Mon, 2014-03-24 at 15:43 +0200, Marcel Apfelbaum wrote:
> On Mon, 2014-03-24 at 13:24 +0000, Peter Maydell wrote:
> > On 24 March 2014 13:23, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > Did you try running gtester without -q or even with -v?  (You can edit
> > > tests/Makefile to do that.)
> > 
> > ...or use make V=1.
> I tried the verbose mode, no g_test_message output.
But g_message can be seen on stdout, is this a valid option?

Thanks,
Marcel

> 
> Thanks,
> Marcel
> 
> > 
> > thanks
> > -- PMM
> > 
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH for-2.0 V4] tests/acpi-test: do not fail if iasl is broken

[Andreas Färber]

Am 24.03.2014 14:24, schrieb Peter Maydell:
> On 24 March 2014 13:23, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> Did you try running gtester without -q or even with -v?  (You can edit
>> tests/Makefile to do that.)
> 
> ...or use make V=1.

V=1 I already tried. (And I also suggested to use that for Travis since
otherwise errors are really hard to identify.)

I also recall a patch from Stefan tweaking gtester options that I've not
(yet) picked up...although that just seemed to make V=1 behavior the
default IIRC?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg