[Qemu-devel] [PATCH 0/2] tests: acpi: improve tests reproducability

[Qemu-devel] [PATCH 0/2] tests: acpi: improve tests reproducability

[Igor Mammedov]

series should help to make tests more reproducable and not depend
on IASL being installed. IASL will be required only is user needs
to get textual diff of mismatching files.

Igor Mammedov (2):
  tests: acpi: do not require IASL for dumping AML blobs
  tests: acpi: do not skip tests when IASL is not installed

 tests/bios-tables-test.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

-- 
2.18.1

[Qemu-devel] [PATCH 1/2] tests: acpi: do not require IASL for dumping AML blobs

[Igor Mammedov]

IASL isn't needed when dumping ACPI tables from guest for
rebuild purposes. So move this part out from IASL branch.

Makes rebuild-expected-aml.sh work without IASL installed
on host.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/bios-tables-test.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index d863233fe9..13bd166b81 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -597,12 +597,10 @@ static void test_acpi_one(const char *params, test_data *data)
     test_acpi_rxsdt_table(data);
     test_acpi_fadt_table(data);
 
-    if (iasl) {
-        if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
-            dump_aml_files(data, true);
-        } else {
-            test_acpi_asl(data);
-        }
+    if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
+        dump_aml_files(data, true);
+    } else if (iasl) {
+        test_acpi_asl(data);
     }
 
     /*
-- 
2.18.1

Re: [Qemu-devel] [PATCH 1/2] tests: acpi: do not require IASL for dumping AML blobs

[Philippe Mathieu-Daudé]

On 7/4/19 10:27 AM, Igor Mammedov wrote:
> IASL isn't needed when dumping ACPI tables from guest for
> rebuild purposes. So move this part out from IASL branch.
> 
> Makes rebuild-expected-aml.sh work without IASL installed
> on host.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

> ---
>  tests/bios-tables-test.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index d863233fe9..13bd166b81 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -597,12 +597,10 @@ static void test_acpi_one(const char *params, test_data *data)
>      test_acpi_rxsdt_table(data);
>      test_acpi_fadt_table(data);
>  
> -    if (iasl) {
> -        if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
> -            dump_aml_files(data, true);
> -        } else {
> -            test_acpi_asl(data);
> -        }
> +    if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
> +        dump_aml_files(data, true);
> +    } else if (iasl) {
> +        test_acpi_asl(data);
>      }
>  
>      /*
> 

Re: [Qemu-devel] [PATCH 1/2] tests: acpi: do not require IASL for dumping AML blobs

[Auger Eric]

Hi Igor,

On 7/4/19 10:27 AM, Igor Mammedov wrote:
> IASL isn't needed when dumping ACPI tables from guest for
> rebuild purposes. So move this part out from IASL branch.
> 
> Makes rebuild-expected-aml.sh work without IASL installed
> on host.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/bios-tables-test.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index d863233fe9..13bd166b81 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -597,12 +597,10 @@ static void test_acpi_one(const char *params, test_data *data)
>      test_acpi_rxsdt_table(data);
>      test_acpi_fadt_table(data);
>  
> -    if (iasl) {
> -        if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
> -            dump_aml_files(data, true);
> -        } else {
> -            test_acpi_asl(data);
> -        }
> +    if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
> +        dump_aml_files(data, true);
> +    } else if (iasl) {
> +        test_acpi_asl(data);
>      }
>  
>      /*
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

[Qemu-devel] [PATCH 2/2] tests: acpi: do not skip tests when IASL is not installed

[Igor Mammedov]

tests do binary comparition so we can check tables without
IASL. Move IASL condition right before decompilation step
and skip it if IASL is not installed.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/bios-tables-test.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 13bd166b81..a356ac3489 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -389,6 +389,14 @@ static void test_acpi_asl(test_data *data)
         all_tables_match = all_tables_match &&
             test_acpi_find_diff_allowed(exp_sdt);
 
+        /*
+         * don't try to decompile if IASL isn't present, in this case user
+         * will just 'get binary file mismatch' warnings and test failure
+         */
+        if (!iasl) {
+            continue;
+        }
+
         err = load_asl(data->tables, sdt);
         asl = normalize_asl(sdt->asl);
 
@@ -431,6 +439,11 @@ static void test_acpi_asl(test_data *data)
         g_string_free(asl, true);
         g_string_free(exp_asl, true);
     }
+    if (!iasl && !all_tables_match) {
+        fprintf(stderr, "to see ASL diff between mismatched files install IASL,"
+                " rebuild QEMU from scratch and re-run tests with V=1"
+                " environment variable set");
+    }
     g_assert(all_tables_match);
 
     free_test_data(&exp_data);
@@ -599,7 +612,7 @@ static void test_acpi_one(const char *params, test_data *data)
 
     if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
         dump_aml_files(data, true);
-    } else if (iasl) {
+    } else {
         test_acpi_asl(data);
     }
 
-- 
2.18.1

Re: [Qemu-devel] [PATCH 2/2] tests: acpi: do not skip tests when IASL is not installed

[Philippe Mathieu-Daudé]

On 7/4/19 10:27 AM, Igor Mammedov wrote:
> tests do binary comparition so we can check tables without
> IASL. Move IASL condition right before decompilation step
> and skip it if IASL is not installed.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/bios-tables-test.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 13bd166b81..a356ac3489 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -389,6 +389,14 @@ static void test_acpi_asl(test_data *data)
>          all_tables_match = all_tables_match &&
>              test_acpi_find_diff_allowed(exp_sdt);
>  
> +        /*
> +         * don't try to decompile if IASL isn't present, in this case user
> +         * will just 'get binary file mismatch' warnings and test failure
> +         */
> +        if (!iasl) {
> +            continue;
> +        }
> +
>          err = load_asl(data->tables, sdt);
>          asl = normalize_asl(sdt->asl);
>  
> @@ -431,6 +439,11 @@ static void test_acpi_asl(test_data *data)
>          g_string_free(asl, true);
>          g_string_free(exp_asl, true);
>      }
> +    if (!iasl && !all_tables_match) {
> +        fprintf(stderr, "to see ASL diff between mismatched files install IASL,"
> +                " rebuild QEMU from scratch and re-run tests with V=1"
> +                " environment variable set");

I guess remember Thomas asked to use g_printerr() instead of fprintf()
in tests/*.c.

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

> +    }
>      g_assert(all_tables_match);
>  
>      free_test_data(&exp_data);
> @@ -599,7 +612,7 @@ static void test_acpi_one(const char *params, test_data *data)
>  
>      if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
>          dump_aml_files(data, true);
> -    } else if (iasl) {
> +    } else {
>          test_acpi_asl(data);
>      }
>  
> 

Re: [Qemu-devel] [PATCH 2/2] tests: acpi: do not skip tests when IASL is not installed

[Igor Mammedov]

On Thu, 4 Jul 2019 11:33:19 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 7/4/19 10:27 AM, Igor Mammedov wrote:
> > tests do binary comparition so we can check tables without
> > IASL. Move IASL condition right before decompilation step
> > and skip it if IASL is not installed.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  tests/bios-tables-test.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> > index 13bd166b81..a356ac3489 100644
> > --- a/tests/bios-tables-test.c
> > +++ b/tests/bios-tables-test.c
> > @@ -389,6 +389,14 @@ static void test_acpi_asl(test_data *data)
> >          all_tables_match = all_tables_match &&
> >              test_acpi_find_diff_allowed(exp_sdt);
> >  
> > +        /*
> > +         * don't try to decompile if IASL isn't present, in this case user
> > +         * will just 'get binary file mismatch' warnings and test failure
> > +         */
> > +        if (!iasl) {
> > +            continue;
> > +        }
> > +
> >          err = load_asl(data->tables, sdt);
> >          asl = normalize_asl(sdt->asl);
> >  
> > @@ -431,6 +439,11 @@ static void test_acpi_asl(test_data *data)
> >          g_string_free(asl, true);
> >          g_string_free(exp_asl, true);
> >      }
> > +    if (!iasl && !all_tables_match) {
> > +        fprintf(stderr, "to see ASL diff between mismatched files install IASL,"
> > +                " rebuild QEMU from scratch and re-run tests with V=1"
> > +                " environment variable set");  
> 
> I guess remember Thomas asked to use g_printerr() instead of fprintf()
whole file uses fprintf().
I'd rather send extra patch on top that consistently changes
test to g_printerr()

> in tests/*.c.
> 
> Anyway,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Thanks!

> 
> > +    }
> >      g_assert(all_tables_match);
> >  
> >      free_test_data(&exp_data);
> > @@ -599,7 +612,7 @@ static void test_acpi_one(const char *params, test_data *data)
> >  
> >      if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
> >          dump_aml_files(data, true);
> > -    } else if (iasl) {
> > +    } else {
> >          test_acpi_asl(data);
> >      }
> >  
> >   
> 

Re: [Qemu-devel] [PATCH 2/2] tests: acpi: do not skip tests when IASL is not installed

[Auger Eric]

Hi,

On 7/4/19 10:27 AM, Igor Mammedov wrote:
> tests do binary comparition so we can check tables without
comparison
> IASL. Move IASL condition right before decompilation step
> and skip it if IASL is not installed.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/bios-tables-test.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 13bd166b81..a356ac3489 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -389,6 +389,14 @@ static void test_acpi_asl(test_data *data)
>          all_tables_match = all_tables_match &&
>              test_acpi_find_diff_allowed(exp_sdt);
>  
> +        /*
> +         * don't try to decompile if IASL isn't present, in this case user
> +         * will just 'get binary file mismatch' warnings and test failure
> +         */
> +        if (!iasl) {
> +            continue;
> +        }
> +
>          err = load_asl(data->tables, sdt);
>          asl = normalize_asl(sdt->asl);
>  
> @@ -431,6 +439,11 @@ static void test_acpi_asl(test_data *data)
>          g_string_free(asl, true);
>          g_string_free(exp_asl, true);
>      }
> +    if (!iasl && !all_tables_match) {
> +        fprintf(stderr, "to see ASL diff between mismatched files install IASL,"
> +                " rebuild QEMU from scratch and re-run tests with V=1"
> +                " environment variable set");
> +    }
>      g_assert(all_tables_match);
>  
>      free_test_data(&exp_data);
> @@ -599,7 +612,7 @@ static void test_acpi_one(const char *params, test_data *data)
>  
>      if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
>          dump_aml_files(data, true);
> -    } else if (iasl) {
> +    } else {
>          test_acpi_asl(data);
>      }
>  
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

[Qemu-devel] [PATCH v2 2/2] tests: acpi: do not skip tests when IASL is not installed

[Igor Mammedov]

tests do binary comparision so we can check tables without
IASL. Move IASL condition right before decompilation step
and skip it if IASL is not installed.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2:
 - fix typo in commit message
     Eric Auger <eric.auger@redhat.com>

 tests/bios-tables-test.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 13bd166b81..a356ac3489 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -389,6 +389,14 @@ static void test_acpi_asl(test_data *data)
         all_tables_match = all_tables_match &&
             test_acpi_find_diff_allowed(exp_sdt);
 
+        /*
+         *  don't try to decompile if IASL isn't present, in this case user
+         * will just 'get binary file mismatch' warnings and test failure
+         */
+        if (!iasl) {
+            continue;
+        }
+
         err = load_asl(data->tables, sdt);
         asl = normalize_asl(sdt->asl);
 
@@ -431,6 +439,11 @@ static void test_acpi_asl(test_data *data)
         g_string_free(asl, true);
         g_string_free(exp_asl, true);
     }
+    if (!iasl && !all_tables_match) {
+        fprintf(stderr, "to see ASL diff between mismatched files install IASL,"
+                " rebuild QEMU from scratch and re-run tests with V=1"
+                " environment variable set");
+    }
     g_assert(all_tables_match);
 
     free_test_data(&exp_data);
@@ -599,7 +612,7 @@ static void test_acpi_one(const char *params, test_data *data)
 
     if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
         dump_aml_files(data, true);
-    } else if (iasl) {
+    } else {
         test_acpi_asl(data);
     }
 
-- 
2.18.1

Re: [Qemu-devel] [PATCH 0/2] tests: acpi: improve tests reproducability

[Michael S. Tsirkin]

On Thu, Jul 04, 2019 at 04:27:34AM -0400, Igor Mammedov wrote:
> 
> series should help to make tests more reproducable and not depend
> on IASL being installed. IASL will be required only is user needs
> to get textual diff of mismatching files.


I like this very much but pls send v3 with both patches so
I can apply it easily.

> Igor Mammedov (2):
>   tests: acpi: do not require IASL for dumping AML blobs
>   tests: acpi: do not skip tests when IASL is not installed
> 
>  tests/bios-tables-test.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> -- 
> 2.18.1