[PATCH 0/5] bios-tables-test: more documentation

[PATCH 0/5] bios-tables-test: more documentation

[Michael S. Tsirkin]

Better error messages, documentation tweaks to help
people follow the process for expected file updates.

Michael S. Tsirkin (5):
  bios-tables-test: tell people how to update
  bios-tables-test: fix up DIFF generation
  bios-tables-test: default diff command
  bios-tables-test: fix path to allowed diff
  rebuild-expected-aml.sh: remind about the process

 tests/qtest/bios-tables-test.c          | 32 ++++++++++++-------------
 tests/data/acpi/rebuild-expected-aml.sh |  7 ++++++
 2 files changed, 23 insertions(+), 16 deletions(-)

-- 
MST

[PATCH 1/5] bios-tables-test: tell people how to update

[Michael S. Tsirkin]

For now just a pointer to the source file.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/qtest/bios-tables-test.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 3ab4872bd7..6b5f24bf62 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -426,7 +426,9 @@ static void test_acpi_asl(test_data *data)
 
         fprintf(stderr,
                 "acpi-test: Warning! %.4s binary file mismatch. "
-                "Actual [aml:%s], Expected [aml:%s].\n",
+                "Actual [aml:%s], Expected [aml:%s].\n"
+                "See source file tests/qtest/bios-tables-test.c "
+                "for instructions on how to update expected files.\n",
                 exp_sdt->aml, sdt->aml_file, exp_sdt->aml_file);
 
         all_tables_match = all_tables_match &&
-- 
MST

[PATCH 2/5] bios-tables-test: fix up DIFF generation

[Michael S. Tsirkin]

Turns out it goes to stdout which is suppressed even with V=1.
Force DIFF output to stderr to make it visible.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/qtest/bios-tables-test.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 6b5f24bf62..c8db2839b2 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -463,13 +463,18 @@ static void test_acpi_asl(test_data *data)
                         "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n",
                         exp_sdt->aml, sdt->asl_file, sdt->aml_file,
                         exp_sdt->asl_file, exp_sdt->aml_file);
+                fflush(stderr);
                 if (getenv("V")) {
                     const char *diff_cmd = getenv("DIFF");
                     if (diff_cmd) {
-                        int ret G_GNUC_UNUSED;
                         char *diff = g_strdup_printf("%s %s %s", diff_cmd,
                             exp_sdt->asl_file, sdt->asl_file);
+                        int out = dup(STDOUT_FILENO);
+                        int ret G_GNUC_UNUSED;
+
+                        dup2(STDERR_FILENO, STDOUT_FILENO);
                         ret = system(diff) ;
+                        dup2(out, STDOUT_FILENO);
                         g_free(diff);
                     } else {
                         fprintf(stderr, "acpi-test: Warning. not showing "
-- 
MST

[PATCH 3/5] bios-tables-test: default diff command

[Michael S. Tsirkin]

Most people probably just want diff -u. So let's use that
as the default.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/qtest/bios-tables-test.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index c8db2839b2..6ec1c5be64 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -465,24 +465,17 @@ static void test_acpi_asl(test_data *data)
                         exp_sdt->asl_file, exp_sdt->aml_file);
                 fflush(stderr);
                 if (getenv("V")) {
-                    const char *diff_cmd = getenv("DIFF");
-                    if (diff_cmd) {
-                        char *diff = g_strdup_printf("%s %s %s", diff_cmd,
-                            exp_sdt->asl_file, sdt->asl_file);
-                        int out = dup(STDOUT_FILENO);
-                        int ret G_GNUC_UNUSED;
+                    const char *diff_env = getenv("DIFF");
+                    const char *diff_cmd = diff_env ? diff_env : "diff -u";
+                    char *diff = g_strdup_printf("%s %s %s", diff_cmd,
+                                                 exp_sdt->asl_file, sdt->asl_file);
+                    int out = dup(STDOUT_FILENO);
+                    int ret G_GNUC_UNUSED;
 
-                        dup2(STDERR_FILENO, STDOUT_FILENO);
-                        ret = system(diff) ;
-                        dup2(out, STDOUT_FILENO);
-                        g_free(diff);
-                    } else {
-                        fprintf(stderr, "acpi-test: Warning. not showing "
-                            "difference since no diff utility is specified. "
-                            "Set 'DIFF' environment variable to a preferred "
-                            "diff utility and run 'make V=1 check' again to "
-                            "see ASL difference.");
-                    }
+                    dup2(STDERR_FILENO, STDOUT_FILENO);
+                    ret = system(diff) ;
+                    dup2(out, STDOUT_FILENO);
+                    g_free(diff);
                 }
             }
         }
-- 
MST

[PATCH 4/5] bios-tables-test: fix path to allowed diff

[Michael S. Tsirkin]

Fixes: 1e8a1fae7464 ("test: Move qtests to a separate directory")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/qtest/bios-tables-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 6ec1c5be64..6535ab7f04 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -21,7 +21,7 @@
  * in binary commit created in step 6):
  *
  * After 1-3 above tests will pass but ignore differences with the expected files.
- * You will also notice that tests/bios-tables-test-allowed-diff.h lists
+ * You will also notice that tests/qtest/bios-tables-test-allowed-diff.h lists
  * a bunch of files. This is your hint that you need to do the below:
  * 4. Run
  *      make check V=1
-- 
MST

[PATCH 5/5] rebuild-expected-aml.sh: remind about the process

[Michael S. Tsirkin]

Remind users of rebuild-expected-aml.sh about the process
to follow. Suppress the warning if allowed file list exists -
that's a big hint user is already aware of the process.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/data/acpi/rebuild-expected-aml.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tests/data/acpi/rebuild-expected-aml.sh b/tests/data/acpi/rebuild-expected-aml.sh
index d44e511533..9cbaab1a4d 100755
--- a/tests/data/acpi/rebuild-expected-aml.sh
+++ b/tests/data/acpi/rebuild-expected-aml.sh
@@ -31,6 +31,13 @@ done
 
 eval `grep SRC_PATH= config-host.mak`
 
+old_allowed_dif=`grep -v -e 'List of comma-separated changed AML files to ignore' ${SRC_PATH}/tests/qtest/bios-tables-test-allowed-diff.h`
+
 echo '/* List of comma-separated changed AML files to ignore */' > ${SRC_PATH}/tests/qtest/bios-tables-test-allowed-diff.h
 
 echo "The files were rebuilt and can be added to git."
+
+if [ -z "$old_allowed_dif" ]; then
+    echo "Note! Please do not commit expected files with source changes"
+    echo "Note! Please follow the process documented in ${SRC_PATH}/tests/qtest/bios-tables-test.c"
+fi
-- 
MST

Re: [PATCH 1/5] bios-tables-test: tell people how to update

[Laurent Vivier]

On 22/01/2020 09:05, Michael S. Tsirkin wrote:
> For now just a pointer to the source file.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  tests/qtest/bios-tables-test.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 3ab4872bd7..6b5f24bf62 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -426,7 +426,9 @@ static void test_acpi_asl(test_data *data)
>  
>          fprintf(stderr,
>                  "acpi-test: Warning! %.4s binary file mismatch. "
> -                "Actual [aml:%s], Expected [aml:%s].\n",
> +                "Actual [aml:%s], Expected [aml:%s].\n"
> +                "See source file tests/qtest/bios-tables-test.c "
> +                "for instructions on how to update expected files.\n",
>                  exp_sdt->aml, sdt->aml_file, exp_sdt->aml_file);
>  
>          all_tables_match = all_tables_match &&
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [PATCH 4/5] bios-tables-test: fix path to allowed diff

[Laurent Vivier]

On 22/01/2020 09:06, Michael S. Tsirkin wrote:
> Fixes: 1e8a1fae7464 ("test: Move qtests to a separate directory")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  tests/qtest/bios-tables-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 6ec1c5be64..6535ab7f04 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -21,7 +21,7 @@
>   * in binary commit created in step 6):
>   *
>   * After 1-3 above tests will pass but ignore differences with the expected files.
> - * You will also notice that tests/bios-tables-test-allowed-diff.h lists
> + * You will also notice that tests/qtest/bios-tables-test-allowed-diff.h lists
>   * a bunch of files. This is your hint that you need to do the below:
>   * 4. Run
>   *      make check V=1
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [PATCH 2/5] bios-tables-test: fix up DIFF generation

[Laurent Vivier]

On 22/01/2020 09:05, Michael S. Tsirkin wrote:
> Turns out it goes to stdout which is suppressed even with V=1.
> Force DIFF output to stderr to make it visible.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  tests/qtest/bios-tables-test.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 6b5f24bf62..c8db2839b2 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -463,13 +463,18 @@ static void test_acpi_asl(test_data *data)
>                          "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n",
>                          exp_sdt->aml, sdt->asl_file, sdt->aml_file,
>                          exp_sdt->asl_file, exp_sdt->aml_file);
> +                fflush(stderr);
>                  if (getenv("V")) {
>                      const char *diff_cmd = getenv("DIFF");
>                      if (diff_cmd) {
> -                        int ret G_GNUC_UNUSED;
>                          char *diff = g_strdup_printf("%s %s %s", diff_cmd,
>                              exp_sdt->asl_file, sdt->asl_file);
> +                        int out = dup(STDOUT_FILENO);
> +                        int ret G_GNUC_UNUSED;
> +
> +                        dup2(STDERR_FILENO, STDOUT_FILENO);
>                          ret = system(diff) ;
> +                        dup2(out, STDOUT_FILENO);

I think you need a "close(out)" here.

Thanks,
Laurent

Re: [PATCH 2/5] bios-tables-test: fix up DIFF generation

[Michael S. Tsirkin]

On Wed, Jan 22, 2020 at 10:00:01AM +0100, Laurent Vivier wrote:
> On 22/01/2020 09:05, Michael S. Tsirkin wrote:
> > Turns out it goes to stdout which is suppressed even with V=1.
> > Force DIFF output to stderr to make it visible.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  tests/qtest/bios-tables-test.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > index 6b5f24bf62..c8db2839b2 100644
> > --- a/tests/qtest/bios-tables-test.c
> > +++ b/tests/qtest/bios-tables-test.c
> > @@ -463,13 +463,18 @@ static void test_acpi_asl(test_data *data)
> >                          "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n",
> >                          exp_sdt->aml, sdt->asl_file, sdt->aml_file,
> >                          exp_sdt->asl_file, exp_sdt->aml_file);
> > +                fflush(stderr);
> >                  if (getenv("V")) {
> >                      const char *diff_cmd = getenv("DIFF");
> >                      if (diff_cmd) {
> > -                        int ret G_GNUC_UNUSED;
> >                          char *diff = g_strdup_printf("%s %s %s", diff_cmd,
> >                              exp_sdt->asl_file, sdt->asl_file);
> > +                        int out = dup(STDOUT_FILENO);
> > +                        int ret G_GNUC_UNUSED;
> > +
> > +                        dup2(STDERR_FILENO, STDOUT_FILENO);
> >                          ret = system(diff) ;
> > +                        dup2(out, STDOUT_FILENO);
> 
> I think you need a "close(out)" here.
> 
> Thanks,
> Laurent

Can't hurt, thanks!

Re: [PATCH 1/5] bios-tables-test: tell people how to update

[Thomas Huth]

On 22/01/2020 09.05, Michael S. Tsirkin wrote:
> For now just a pointer to the source file.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  tests/qtest/bios-tables-test.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 3ab4872bd7..6b5f24bf62 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -426,7 +426,9 @@ static void test_acpi_asl(test_data *data)
>  
>          fprintf(stderr,
>                  "acpi-test: Warning! %.4s binary file mismatch. "
> -                "Actual [aml:%s], Expected [aml:%s].\n",
> +                "Actual [aml:%s], Expected [aml:%s].\n"
> +                "See source file tests/qtest/bios-tables-test.c "
> +                "for instructions on how to update expected files.\n",
>                  exp_sdt->aml, sdt->aml_file, exp_sdt->aml_file);
>  
>          all_tables_match = all_tables_match &&
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 4/5] bios-tables-test: fix path to allowed diff

[Thomas Huth]

On 22/01/2020 09.06, Michael S. Tsirkin wrote:
> Fixes: 1e8a1fae7464 ("test: Move qtests to a separate directory")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  tests/qtest/bios-tables-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 6ec1c5be64..6535ab7f04 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -21,7 +21,7 @@
>   * in binary commit created in step 6):
>   *
>   * After 1-3 above tests will pass but ignore differences with the expected files.
> - * You will also notice that tests/bios-tables-test-allowed-diff.h lists
> + * You will also notice that tests/qtest/bios-tables-test-allowed-diff.h lists
>   * a bunch of files. This is your hint that you need to do the below:
>   * 4. Run
>   *      make check V=1
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>