[Qemu-devel] [PATCH 0/2] acpi unit-test: rebuild aml files functionality

[Qemu-devel] [PATCH 0/2] acpi unit-test: rebuild aml files functionality

[Marcel Apfelbaum]

Acpi unit-test will fail every time the acpi tables change.

The series adds a script that rebuilds the expected aml files, so the test
will pass. It also validates the modifications.

The acpi unit test will rebuild the aml table if ACPI_REBUILD_AML
environment variable is set.

Marcel Apfelbaum (2):
  acpi unit-test: added script to rebuild the expected aml files
  acpi unit-test: hook to rebuild expected aml files

 tests/acpi-test-data/rebuild-expected-aml.sh | 36 ++++++++++++++++++++++++++++
 tests/acpi-test.c                            | 30 +++++++++++++++++++----
 2 files changed, 61 insertions(+), 5 deletions(-)
 create mode 100755 tests/acpi-test-data/rebuild-expected-aml.sh

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] acpi unit-test: added script to rebuild the expected aml files

[Marcel Apfelbaum]

Acpi unit-test will fail every time the acpi tables change.
This script rebuilds the expected aml files, so the test
will pass. It also validates the modifications.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 tests/acpi-test-data/rebuild-expected-aml.sh | 36 ++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100755 tests/acpi-test-data/rebuild-expected-aml.sh

diff --git a/tests/acpi-test-data/rebuild-expected-aml.sh b/tests/acpi-test-data/rebuild-expected-aml.sh
new file mode 100755
index 0000000..8039718
--- /dev/null
+++ b/tests/acpi-test-data/rebuild-expected-aml.sh
@@ -0,0 +1,36 @@
+#! /bin/bash
+
+#
+# Rebuild expected AML files for acpi unit-test 
+#
+# Copyright (c) 2013 Red Hat Inc.
+#
+# Authors:
+#  Marcel Apfelbaum <marcel.a@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPLv2.
+# See the COPYING.LIB file in the top-level directory.
+
+qemu=
+
+if [ -e x86_64-softmmu/qemu-system-x86_64 ]; then
+    qemu="x86_64-softmmu/qemu-system-x86_64"
+elif [ -e i386-softmmu/qemu-system-i386 ]; then
+    qemu="i386-softmmu/qemu-system-i386"
+else
+    echo "Qemu executable qemu-system-x86_64 or qemu-system-i386 is required!"
+    echo "Run this script from the build directory."
+    exit -1;
+fi
+
+if [ ! -e "tests/acpi-test" ]; then
+    echo "Test: acpi-test is required! Run make check before this script."
+    echo "Run this script from the build directory."
+    exit -1;
+fi
+
+ACPI_REBUILD_AML=y QTEST_QEMU_BINARY=$qemu tests/acpi-test
+
+echo "The files were rebuild and can be added to git."
+echo "However, if new files were created, please copy them manually" \
+     "to tests/acpi-test-data/pc/ or tests/acpi-test-data/q35/ ."
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] acpi unit-test: added script to rebuild the expected aml files

[Michael S. Tsirkin]

On Thu, Dec 26, 2013 at 12:44:10PM +0200, Marcel Apfelbaum wrote:
> Acpi unit-test will fail every time the acpi tables change.
> This script rebuilds the expected aml files, so the test
> will pass. It also validates the modifications.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  tests/acpi-test-data/rebuild-expected-aml.sh | 36 ++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100755 tests/acpi-test-data/rebuild-expected-aml.sh
> 
> diff --git a/tests/acpi-test-data/rebuild-expected-aml.sh b/tests/acpi-test-data/rebuild-expected-aml.sh
> new file mode 100755
> index 0000000..8039718
> --- /dev/null
> +++ b/tests/acpi-test-data/rebuild-expected-aml.sh
> @@ -0,0 +1,36 @@
> +#! /bin/bash
> +
> +#
> +# Rebuild expected AML files for acpi unit-test 
> +#
> +# Copyright (c) 2013 Red Hat Inc.
> +#
> +# Authors:
> +#  Marcel Apfelbaum <marcel.a@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPLv2.
> +# See the COPYING.LIB file in the top-level directory.
> +
> +qemu=
> +
> +if [ -e x86_64-softmmu/qemu-system-x86_64 ]; then
> +    qemu="x86_64-softmmu/qemu-system-x86_64"
> +elif [ -e i386-softmmu/qemu-system-i386 ]; then
> +    qemu="i386-softmmu/qemu-system-i386"
> +else
> +    echo "Qemu executable qemu-system-x86_64 or qemu-system-i386 is required!"

maybe add "run make to build the executable"

> +    echo "Run this script from the build directory."
> +    exit -1;
> +fi
> +
> +if [ ! -e "tests/acpi-test" ]; then
> +    echo "Test: acpi-test is required! Run make check before this script."
> +    echo "Run this script from the build directory."
> +    exit -1;

you want exit 1 or some other positive value.

> +fi
> +
> +ACPI_REBUILD_AML=y QTEST_QEMU_BINARY=$qemu tests/acpi-test
> +
> +echo "The files were rebuild and can be added to git."

rebuilt

> +echo "However, if new files were created, please copy them manually" \
> +     "to tests/acpi-test-data/pc/ or tests/acpi-test-data/q35/ ."
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] acpi unit-test: added script to rebuild the expected aml files

[Marcel Apfelbaum]

On Thu, 2013-12-26 at 12:53 +0200, Michael S. Tsirkin wrote:
> On Thu, Dec 26, 2013 at 12:44:10PM +0200, Marcel Apfelbaum wrote:
> > Acpi unit-test will fail every time the acpi tables change.
> > This script rebuilds the expected aml files, so the test
> > will pass. It also validates the modifications.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  tests/acpi-test-data/rebuild-expected-aml.sh | 36 ++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >  create mode 100755 tests/acpi-test-data/rebuild-expected-aml.sh
> > 
> > diff --git a/tests/acpi-test-data/rebuild-expected-aml.sh b/tests/acpi-test-data/rebuild-expected-aml.sh
> > new file mode 100755
> > index 0000000..8039718
> > --- /dev/null
> > +++ b/tests/acpi-test-data/rebuild-expected-aml.sh
> > @@ -0,0 +1,36 @@
> > +#! /bin/bash
> > +
> > +#
> > +# Rebuild expected AML files for acpi unit-test 
> > +#
> > +# Copyright (c) 2013 Red Hat Inc.
> > +#
> > +# Authors:
> > +#  Marcel Apfelbaum <marcel.a@redhat.com>
> > +#
> > +# This work is licensed under the terms of the GNU GPLv2.
> > +# See the COPYING.LIB file in the top-level directory.
> > +
> > +qemu=
> > +
> > +if [ -e x86_64-softmmu/qemu-system-x86_64 ]; then
> > +    qemu="x86_64-softmmu/qemu-system-x86_64"
> > +elif [ -e i386-softmmu/qemu-system-i386 ]; then
> > +    qemu="i386-softmmu/qemu-system-i386"
> > +else
> > +    echo "Qemu executable qemu-system-x86_64 or qemu-system-i386 is required!"
> 
> maybe add "run make to build the executable"
OK
> 
> > +    echo "Run this script from the build directory."
> > +    exit -1;
> > +fi
> > +
> > +if [ ! -e "tests/acpi-test" ]; then
> > +    echo "Test: acpi-test is required! Run make check before this script."
> > +    echo "Run this script from the build directory."
> > +    exit -1;
> 
> you want exit 1 or some other positive value.
Sure

> 
> > +fi
> > +
> > +ACPI_REBUILD_AML=y QTEST_QEMU_BINARY=$qemu tests/acpi-test
> > +
> > +echo "The files were rebuild and can be added to git."
> 
> rebuilt
Sure,

Thanks,
Marcel

> 
> > +echo "However, if new files were created, please copy them manually" \
> > +     "to tests/acpi-test-data/pc/ or tests/acpi-test-data/q35/ ."
> > -- 
> > 1.8.3.1

[Qemu-devel] [PATCH 2/2] acpi unit-test: hook to rebuild expected aml files

[Marcel Apfelbaum]

When running the test with ACPI_REBUILD_AML=y environment
variable, the test will rebuild and validate the expected aml
files.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 tests/acpi-test.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/tests/acpi-test.c b/tests/acpi-test.c
index 954d9b9..2b8545d 100644
--- a/tests/acpi-test.c
+++ b/tests/acpi-test.c
@@ -13,6 +13,7 @@
 #include <string.h>
 #include <stdio.h>
 #include <glib.h>
+#include <glib/gstdio.h>
 #include "qemu-common.h"
 #include "libqtest.h"
 #include "qemu/compiler.h"
@@ -21,6 +22,8 @@
 #define MACHINE_PC "pc"
 #define MACHINE_Q35 "q35"
 
+#define ACPI_REBUILD_EXPECTED_AML "ACPI_REBUILD_AML"
+
 /* DSDT and SSDTs format */
 typedef struct {
     AcpiTableHeader header;
@@ -363,10 +366,11 @@ static void test_acpi_ssdt_tables(test_data *data)
     }
 }
 
-static void dump_aml_files(test_data *data)
+static void dump_aml_files(test_data *data, bool rebuild)
 {
     AcpiSdtTable *sdt;
     GError *error = NULL;
+    gchar *aml_file = NULL;
     gint fd;
     int i;
 
@@ -374,12 +378,24 @@ static void dump_aml_files(test_data *data)
         sdt = &g_array_index(data->ssdt_tables, AcpiSdtTable, i);
         g_assert(sdt->aml);
 
-        fd = g_file_open_tmp("aml-XXXXXX", &sdt->aml_file, &error);
-        g_assert_no_error(error);
+        if (rebuild) {
+            aml_file = g_strdup_printf("%s/%s/%.4s", data_dir, data->machine,
+                                       (gchar *)&sdt->header.signature);
+            fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
+                        S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
+        } else {
+            fd = g_file_open_tmp("aml-XXXXXX", &sdt->aml_file, &error);
+            g_assert_no_error(error);
+        }
+        g_assert(fd >= 0);
 
         write(fd, sdt, sizeof(AcpiTableHeader));
         write(fd, sdt->aml, sdt->aml_len);
         close(fd);
+
+        if (aml_file) {
+            g_free(aml_file);
+        }
     }
 }
 
@@ -487,7 +503,7 @@ static void test_acpi_asl(test_data *data)
 
     memset(&exp_data, 0, sizeof(exp_data));
     exp_data.ssdt_tables = load_expected_aml(data);
-    dump_aml_files(data);
+    dump_aml_files(data, false);
     for (i = 0; i < data->ssdt_tables->len; ++i) {
         GString *asl, *exp_asl;
 
@@ -553,7 +569,11 @@ static void test_acpi_one(const char *params, test_data *data)
     test_acpi_ssdt_tables(data);
 
     if (iasl) {
-        test_acpi_asl(data);
+        if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
+            dump_aml_files(data, true);
+        } else {
+            test_acpi_asl(data);
+        }
     }
 
     qtest_quit(global_qtest);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] acpi unit-test: hook to rebuild expected aml files

[Michael S. Tsirkin]

On Thu, Dec 26, 2013 at 12:44:11PM +0200, Marcel Apfelbaum wrote:
> When running the test with ACPI_REBUILD_AML=y environment
> variable, the test will rebuild and validate the expected aml
> files.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  tests/acpi-test.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/acpi-test.c b/tests/acpi-test.c
> index 954d9b9..2b8545d 100644
> --- a/tests/acpi-test.c
> +++ b/tests/acpi-test.c
> @@ -13,6 +13,7 @@
>  #include <string.h>
>  #include <stdio.h>
>  #include <glib.h>
> +#include <glib/gstdio.h>
>  #include "qemu-common.h"
>  #include "libqtest.h"
>  #include "qemu/compiler.h"
> @@ -21,6 +22,8 @@
>  #define MACHINE_PC "pc"
>  #define MACHINE_Q35 "q35"
>  
> +#define ACPI_REBUILD_EXPECTED_AML "ACPI_REBUILD_AML"
> +

Put TEST somewhere in the name please. Maybe as a prefix:
TEST_ACPI_REBUILD_AML

>  /* DSDT and SSDTs format */
>  typedef struct {
>      AcpiTableHeader header;
> @@ -363,10 +366,11 @@ static void test_acpi_ssdt_tables(test_data *data)
>      }
>  }
>  
> -static void dump_aml_files(test_data *data)
> +static void dump_aml_files(test_data *data, bool rebuild)
>  {
>      AcpiSdtTable *sdt;
>      GError *error = NULL;
> +    gchar *aml_file = NULL;
>      gint fd;
>      int i;
>  
> @@ -374,12 +378,24 @@ static void dump_aml_files(test_data *data)
>          sdt = &g_array_index(data->ssdt_tables, AcpiSdtTable, i);
>          g_assert(sdt->aml);
>  
> -        fd = g_file_open_tmp("aml-XXXXXX", &sdt->aml_file, &error);
> -        g_assert_no_error(error);
> +        if (rebuild) {
> +            aml_file = g_strdup_printf("%s/%s/%.4s", data_dir, data->machine,
> +                                       (gchar *)&sdt->header.signature);

Hmm will this work if there are many SSDTs?
They all have same signature,

> +            fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
> +                        S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
> +        } else {
> +            fd = g_file_open_tmp("aml-XXXXXX", &sdt->aml_file, &error);
> +            g_assert_no_error(error);
> +        }
> +        g_assert(fd >= 0);
>  
>          write(fd, sdt, sizeof(AcpiTableHeader));
>          write(fd, sdt->aml, sdt->aml_len);
>          close(fd);
> +
> +        if (aml_file) {
> +            g_free(aml_file);
> +        }
>      }
>  }
>  
> @@ -487,7 +503,7 @@ static void test_acpi_asl(test_data *data)
>  
>      memset(&exp_data, 0, sizeof(exp_data));
>      exp_data.ssdt_tables = load_expected_aml(data);
> -    dump_aml_files(data);
> +    dump_aml_files(data, false);
>      for (i = 0; i < data->ssdt_tables->len; ++i) {
>          GString *asl, *exp_asl;
>  
> @@ -553,7 +569,11 @@ static void test_acpi_one(const char *params, test_data *data)
>      test_acpi_ssdt_tables(data);
>  
>      if (iasl) {
> -        test_acpi_asl(data);
> +        if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
> +            dump_aml_files(data, true);
> +        } else {
> +            test_acpi_asl(data);
> +        }
>      }
>  
>      qtest_quit(global_qtest);
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] acpi unit-test: hook to rebuild expected aml files

[Marcel Apfelbaum]

On Thu, 2013-12-26 at 12:56 +0200, Michael S. Tsirkin wrote:
> On Thu, Dec 26, 2013 at 12:44:11PM +0200, Marcel Apfelbaum wrote:
> > When running the test with ACPI_REBUILD_AML=y environment
> > variable, the test will rebuild and validate the expected aml
> > files.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  tests/acpi-test.c | 30 +++++++++++++++++++++++++-----
> >  1 file changed, 25 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tests/acpi-test.c b/tests/acpi-test.c
> > index 954d9b9..2b8545d 100644
> > --- a/tests/acpi-test.c
> > +++ b/tests/acpi-test.c
> > @@ -13,6 +13,7 @@
> >  #include <string.h>
> >  #include <stdio.h>
> >  #include <glib.h>
> > +#include <glib/gstdio.h>
> >  #include "qemu-common.h"
> >  #include "libqtest.h"
> >  #include "qemu/compiler.h"
> > @@ -21,6 +22,8 @@
> >  #define MACHINE_PC "pc"
> >  #define MACHINE_Q35 "q35"
> >  
> > +#define ACPI_REBUILD_EXPECTED_AML "ACPI_REBUILD_AML"
> > +
> 
> Put TEST somewhere in the name please. Maybe as a prefix:
> TEST_ACPI_REBUILD_AML
Sure

> 
> >  /* DSDT and SSDTs format */
> >  typedef struct {
> >      AcpiTableHeader header;
> > @@ -363,10 +366,11 @@ static void test_acpi_ssdt_tables(test_data *data)
> >      }
> >  }
> >  
> > -static void dump_aml_files(test_data *data)
> > +static void dump_aml_files(test_data *data, bool rebuild)
> >  {
> >      AcpiSdtTable *sdt;
> >      GError *error = NULL;
> > +    gchar *aml_file = NULL;
> >      gint fd;
> >      int i;
> >  
> > @@ -374,12 +378,24 @@ static void dump_aml_files(test_data *data)
> >          sdt = &g_array_index(data->ssdt_tables, AcpiSdtTable, i);
> >          g_assert(sdt->aml);
> >  
> > -        fd = g_file_open_tmp("aml-XXXXXX", &sdt->aml_file, &error);
> > -        g_assert_no_error(error);
> > +        if (rebuild) {
> > +            aml_file = g_strdup_printf("%s/%s/%.4s", data_dir, data->machine,
> > +                                       (gchar *)&sdt->header.signature);
> 
> Hmm will this work if there are many SSDTs?
> They all have same signature,
>From what I understand there is only one ssdt table named SSDT,
all others are named differently: "APIC" "HPET".
I think multiple SSDTS have different signatures, even in my laptop
they are called: SSDT1, SSDT2, SSDT3,...

So, even if the SPEC allows it, for qemu we can use different signatures
for ssdt tables. (it will complicate the code unnecessary)

Thanks,
Marcel

> 
> > +            fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
> > +                        S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
> > +        } else {
> > +            fd = g_file_open_tmp("aml-XXXXXX", &sdt->aml_file, &error);
> > +            g_assert_no_error(error);
> > +        }
> > +        g_assert(fd >= 0);
> >  
> >          write(fd, sdt, sizeof(AcpiTableHeader));
> >          write(fd, sdt->aml, sdt->aml_len);
> >          close(fd);
> > +
> > +        if (aml_file) {
> > +            g_free(aml_file);
> > +        }
> >      }
> >  }
> >  
> > @@ -487,7 +503,7 @@ static void test_acpi_asl(test_data *data)
> >  
> >      memset(&exp_data, 0, sizeof(exp_data));
> >      exp_data.ssdt_tables = load_expected_aml(data);
> > -    dump_aml_files(data);
> > +    dump_aml_files(data, false);
> >      for (i = 0; i < data->ssdt_tables->len; ++i) {
> >          GString *asl, *exp_asl;
> >  
> > @@ -553,7 +569,11 @@ static void test_acpi_one(const char *params, test_data *data)
> >      test_acpi_ssdt_tables(data);
> >  
> >      if (iasl) {
> > -        test_acpi_asl(data);
> > +        if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
> > +            dump_aml_files(data, true);
> > +        } else {
> > +            test_acpi_asl(data);
> > +        }
> >      }
> >  
> >      qtest_quit(global_qtest);
> > -- 
> > 1.8.3.1