[PULL,03/15] tests/acpi/bios-tables-test: do not write new blobs unless there are changes

Commit Message

Michael S. Tsirkin Dec. 1, 2023, 5:15 p.m. UTC

From: Ani Sinha <[e-mail]>

When dumping table blobs using rebuild-expected-aml.sh, table blobs from all
test variants are dumped regardless of whether there are any actual changes to
the tables or not. This creates lot of new files for various test variants that
are not part of the git repository. This is because we do not check in all table
blobs for all test variants into the repository. Only those blobs for those
variants that are different from the generic test-variant agnostic blob are
checked in.

This change makes the test smarter by checking if at all there are any changes
in the tables from the checked-in gold master blobs and take actions
accordingly.

When there are no changes:
 - No new table blobs would be written.
 - Existing table blobs will be refreshed (git diff will show no changes).
When there are changes:
 - New table blob files will be dumped.
 - Existing table blobs will be refreshed (git diff will show that the files
   changed, asl diff will show the actual changes).
When new tables are introduced:
 - Zero byte empty file blobs for new tables as instructed in the header of
   bios-tables-test.c will be regenerated to actual table blobs.

This would make analyzing changes to tables less confusing and there would
be no need to clean useless untracked files when there are no table changes.

CC: [e-mail]
Signed-off-by: Ani Sinha <[e-mail]>
Message-Id: <[e-mail]>
Reviewed-by: Michael S. Tsirkin <[e-mail]>
Signed-off-by: Michael S. Tsirkin <[e-mail]>
---
 tests/qtest/bios-tables-test.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

Ani Sinha Dec. 1, 2023, 5:27 p.m. UTC | #1

> On 01-Dec-2023, at 10:45 PM, Michael S. Tsirkin <[e-mail]> wrote:
> 
> From: Ani Sinha <[e-mail]>
> 
> When dumping table blobs using rebuild-expected-aml.sh, table blobs from all
> test variants are dumped regardless of whether there are any actual changes to
> the tables or not. This creates lot of new files for various test variants that
> are not part of the git repository. This is because we do not check in all table
> blobs for all test variants into the repository. Only those blobs for those
> variants that are different from the generic test-variant agnostic blob are
> checked in.
> 
> This change makes the test smarter by checking if at all there are any changes
> in the tables from the checked-in gold master blobs and take actions
> accordingly.
> 
> When there are no changes:
> - No new table blobs would be written.
> - Existing table blobs will be refreshed (git diff will show no changes).
> When there are changes:
> - New table blob files will be dumped.
> - Existing table blobs will be refreshed (git diff will show that the files
>   changed, asl diff will show the actual changes).
> When new tables are introduced:
> - Zero byte empty file blobs for new tables as instructed in the header of
>   bios-tables-test.c will be regenerated to actual table blobs.
> 
> This would make analyzing changes to tables less confusing and there would
> be no need to clean useless untracked files when there are no table changes.
> 
> CC: [e-mail]
> Signed-off-by: Ani Sinha <[e-mail]>
> Message-Id: <[e-mail]>
> Reviewed-by: Michael S. Tsirkin <[e-mail]>
> Signed-off-by: Michael S. Tsirkin <[e-mail]>

You missed DanPB and Igor’s tags :(

> ---
> tests/qtest/bios-tables-test.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 71af5cf69f..fe6a9a8563 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -112,6 +112,7 @@ static const char *iasl;
> #endif
> 
> static int verbosity_level;
> +static GArray *load_expected_aml(test_data *data);
> 
> static bool compare_signature(const AcpiSdtTable *sdt, const char *signature)
> {
> @@ -244,21 +245,32 @@ static void test_acpi_fadt_table(test_data *data)
> 
> static void dump_aml_files(test_data *data, bool rebuild)
> {
> -    AcpiSdtTable *sdt;
> +    AcpiSdtTable *sdt, *exp_sdt;
>     GError *error = NULL;
>     gchar *aml_file = NULL;
> +    test_data exp_data = {};
>     gint fd;
>     ssize_t ret;
>     int i;
> 
> +    exp_data.tables = load_expected_aml(data);
>     for (i = 0; i < data->tables->len; ++i) {
>         const char *ext = data->variant ? data->variant : "";
>         sdt = &g_array_index(data->tables, AcpiSdtTable, i);
> +        exp_sdt = &g_array_index(exp_data.tables, AcpiSdtTable, i);
>         g_assert(sdt->aml);
> +        g_assert(exp_sdt->aml);
> 
>         if (rebuild) {
>             aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine,
>                                        sdt->aml, ext);
> +            if (!g_file_test(aml_file, G_FILE_TEST_EXISTS) &&
> +                sdt->aml_len == exp_sdt->aml_len &&
> +                !memcmp(sdt->aml, exp_sdt->aml, sdt->aml_len)) {
> +                /* identical tables, no need to write new files */
> +                g_free(aml_file);
> +                continue;
> +            }
>             fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
>                         S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
>             if (fd < 0) {
> -- 
> MST
>

Michael S. Tsirkin Dec. 2, 2023, 8:59 p.m. UTC | #2

On Fri, Dec 01, 2023 at 10:57:04PM +0530, Ani Sinha wrote:
> 
> 
> > On 01-Dec-2023, at 10:45 PM, Michael S. Tsirkin <[e-mail]> wrote:
> > 
> > From: Ani Sinha <[e-mail]>
> > 
> > When dumping table blobs using rebuild-expected-aml.sh, table blobs from all
> > test variants are dumped regardless of whether there are any actual changes to
> > the tables or not. This creates lot of new files for various test variants that
> > are not part of the git repository. This is because we do not check in all table
> > blobs for all test variants into the repository. Only those blobs for those
> > variants that are different from the generic test-variant agnostic blob are
> > checked in.
> > 
> > This change makes the test smarter by checking if at all there are any changes
> > in the tables from the checked-in gold master blobs and take actions
> > accordingly.
> > 
> > When there are no changes:
> > - No new table blobs would be written.
> > - Existing table blobs will be refreshed (git diff will show no changes).
> > When there are changes:
> > - New table blob files will be dumped.
> > - Existing table blobs will be refreshed (git diff will show that the files
> >   changed, asl diff will show the actual changes).
> > When new tables are introduced:
> > - Zero byte empty file blobs for new tables as instructed in the header of
> >   bios-tables-test.c will be regenerated to actual table blobs.
> > 
> > This would make analyzing changes to tables less confusing and there would
> > be no need to clean useless untracked files when there are no table changes.
> > 
> > CC: [e-mail]
> > Signed-off-by: Ani Sinha <[e-mail]>
> > Message-Id: <[e-mail]>
> > Reviewed-by: Michael S. Tsirkin <[e-mail]>
> > Signed-off-by: Michael S. Tsirkin <[e-mail]>
> 
> You missed DanPB and Igor’s tags :(

fixed now, thanks!

> > ---
> > tests/qtest/bios-tables-test.c | 14 +++++++++++++-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > index 71af5cf69f..fe6a9a8563 100644
> > --- a/tests/qtest/bios-tables-test.c
> > +++ b/tests/qtest/bios-tables-test.c
> > @@ -112,6 +112,7 @@ static const char *iasl;
> > #endif
> > 
> > static int verbosity_level;
> > +static GArray *load_expected_aml(test_data *data);
> > 
> > static bool compare_signature(const AcpiSdtTable *sdt, const char *signature)
> > {
> > @@ -244,21 +245,32 @@ static void test_acpi_fadt_table(test_data *data)
> > 
> > static void dump_aml_files(test_data *data, bool rebuild)
> > {
> > -    AcpiSdtTable *sdt;
> > +    AcpiSdtTable *sdt, *exp_sdt;
> >     GError *error = NULL;
> >     gchar *aml_file = NULL;
> > +    test_data exp_data = {};
> >     gint fd;
> >     ssize_t ret;
> >     int i;
> > 
> > +    exp_data.tables = load_expected_aml(data);
> >     for (i = 0; i < data->tables->len; ++i) {
> >         const char *ext = data->variant ? data->variant : "";
> >         sdt = &g_array_index(data->tables, AcpiSdtTable, i);
> > +        exp_sdt = &g_array_index(exp_data.tables, AcpiSdtTable, i);
> >         g_assert(sdt->aml);
> > +        g_assert(exp_sdt->aml);
> > 
> >         if (rebuild) {
> >             aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine,
> >                                        sdt->aml, ext);
> > +            if (!g_file_test(aml_file, G_FILE_TEST_EXISTS) &&
> > +                sdt->aml_len == exp_sdt->aml_len &&
> > +                !memcmp(sdt->aml, exp_sdt->aml, sdt->aml_len)) {
> > +                /* identical tables, no need to write new files */
> > +                g_free(aml_file);
> > +                continue;
> > +            }
> >             fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
> >                         S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
> >             if (fd < 0) {
> > -- 
> > MST
> >

Igor Mammedov Jan. 24, 2024, 8:58 a.m. UTC | #3

On Fri, 1 Dec 2023 12:15:16 -0500
"Michael S. Tsirkin" <[e-mail]> wrote:

> From: Ani Sinha <[e-mail]>
> 
> When dumping table blobs using rebuild-expected-aml.sh, table blobs from all
> test variants are dumped regardless of whether there are any actual changes to
> the tables or not. This creates lot of new files for various test variants that
> are not part of the git repository. This is because we do not check in all table
> blobs for all test variants into the repository. Only those blobs for those
> variants that are different from the generic test-variant agnostic blob are
> checked in.
> 
> This change makes the test smarter by checking if at all there are any changes
> in the tables from the checked-in gold master blobs and take actions
> accordingly.
> 
> When there are no changes:
>  - No new table blobs would be written.
>  - Existing table blobs will be refreshed (git diff will show no changes).
> When there are changes:
>  - New table blob files will be dumped.
>  - Existing table blobs will be refreshed (git diff will show that the files
>    changed, asl diff will show the actual changes).

> When new tables are introduced:
>  - Zero byte empty file blobs for new tables as instructed in the header of
>    bios-tables-test.c will be regenerated to actual table blobs.

Ani,

what previously worked is the is there weren't _any_ expected blobs
(exact match/fallback) found a new table would be dumped.
So then later 'git status' would show a list of new files. 
With this commit it's however not dumping new files files,
and explodes at
#5  0x000055555556808e in load_expected_aml (data=0x7fffffffd7b0) at tests/qtest/bios-tables-test.c:414
#6  0x00005555555676b0 in dump_aml_files (data=0x7fffffffd7b0, rebuild=true) at tests/qtest/bios-tables-test.c:256

sure it can be worked around by manually creating empty files
for expected files but if it's a new machine type, one has to create
a bunch of them basically running multiple iterations of rebuild in V=2
mode to see what's missing.
(IMHO cure turned out to be worse than illness)

for reproducing create a test for other than default machine type, ex:

    test_data data = {                                                           
        .machine = "pc-i440fx-2.0",                                              
        .variant = ".pc_legacy",                                                              
    }; 


is it possible to fix it so that one doesn't have to create empty files manually?

> 
> This would make analyzing changes to tables less confusing and there would
> be no need to clean useless untracked files when there are no table changes.
> 
> CC: [e-mail]
> Signed-off-by: Ani Sinha <[e-mail]>
> Message-Id: <[e-mail]>
> Reviewed-by: Michael S. Tsirkin <[e-mail]>
> Signed-off-by: Michael S. Tsirkin <[e-mail]>
> ---
>  tests/qtest/bios-tables-test.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 71af5cf69f..fe6a9a8563 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -112,6 +112,7 @@ static const char *iasl;
>  #endif
>  
>  static int verbosity_level;
> +static GArray *load_expected_aml(test_data *data);
>  
>  static bool compare_signature(const AcpiSdtTable *sdt, const char *signature)
>  {
> @@ -244,21 +245,32 @@ static void test_acpi_fadt_table(test_data *data)
>  
>  static void dump_aml_files(test_data *data, bool rebuild)
>  {
> -    AcpiSdtTable *sdt;
> +    AcpiSdtTable *sdt, *exp_sdt;
>      GError *error = NULL;
>      gchar *aml_file = NULL;
> +    test_data exp_data = {};
>      gint fd;
>      ssize_t ret;
>      int i;
>  
> +    exp_data.tables = load_expected_aml(data);
>      for (i = 0; i < data->tables->len; ++i) {
>          const char *ext = data->variant ? data->variant : "";
>          sdt = &g_array_index(data->tables, AcpiSdtTable, i);
> +        exp_sdt = &g_array_index(exp_data.tables, AcpiSdtTable, i);
>          g_assert(sdt->aml);
> +        g_assert(exp_sdt->aml);
>  
>          if (rebuild) {
>              aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine,
>                                         sdt->aml, ext);
> +            if (!g_file_test(aml_file, G_FILE_TEST_EXISTS) &&
> +                sdt->aml_len == exp_sdt->aml_len &&
> +                !memcmp(sdt->aml, exp_sdt->aml, sdt->aml_len)) {
> +                /* identical tables, no need to write new files */
> +                g_free(aml_file);
> +                continue;
> +            }
>              fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
>                          S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
>              if (fd < 0) {

Ani Sinha Jan. 24, 2024, 9:47 a.m. UTC | #4

> On 24-Jan-2024, at 14:28, Igor Mammedov <[e-mail]> wrote:
> 
> On Fri, 1 Dec 2023 12:15:16 -0500
> "Michael S. Tsirkin" <[e-mail]> wrote:
> 
>> From: Ani Sinha <[e-mail]>
>> 
>> When dumping table blobs using rebuild-expected-aml.sh, table blobs from all
>> test variants are dumped regardless of whether there are any actual changes to
>> the tables or not. This creates lot of new files for various test variants that
>> are not part of the git repository. This is because we do not check in all table
>> blobs for all test variants into the repository. Only those blobs for those
>> variants that are different from the generic test-variant agnostic blob are
>> checked in.
>> 
>> This change makes the test smarter by checking if at all there are any changes
>> in the tables from the checked-in gold master blobs and take actions
>> accordingly.
>> 
>> When there are no changes:
>> - No new table blobs would be written.
>> - Existing table blobs will be refreshed (git diff will show no changes).
>> When there are changes:
>> - New table blob files will be dumped.
>> - Existing table blobs will be refreshed (git diff will show that the files
>>   changed, asl diff will show the actual changes).
> 
>> When new tables are introduced:
>> - Zero byte empty file blobs for new tables as instructed in the header of
>>   bios-tables-test.c will be regenerated to actual table blobs.
> 
> Ani,
> 
> what previously worked is the is there weren't _any_ expected blobs
> (exact match/fallback) found a new table would be dumped.
> So then later 'git status' would show a list of new files.

Yes this was the original complaint from Peter. Lots of new files were generated when basically they were identical but differed in naming.

> With this commit it's however not dumping new files files,
> and explodes at
> #5  0x000055555556808e in load_expected_aml (data=0x7fffffffd7b0) at tests/qtest/bios-tables-test.c:414
> #6  0x00005555555676b0 in dump_aml_files (data=0x7fffffffd7b0, rebuild=true) at tests/qtest/bios-tables-test.c:256
> 
> sure it can be worked around by manually creating empty files
> for expected files

Yes this has already been documented in the commit log.

> but if it's a new machine type, one has to create
> a bunch of them basically running multiple iterations of rebuild in V=2
> mode to see what's missing.
> (IMHO cure turned out to be worse than illness)

I don’t know how common it is to create new machines. 

> 
> for reproducing create a test for other than default machine type, ex:
> 
>    test_data data = {                                                           
>        .machine = "pc-i440fx-2.0",                                              
>        .variant = ".pc_legacy",                                                              
>    }; 
> 
> 
> is it possible to fix it so that one doesn't have to create empty files manually?
> 
>> 
>> This would make analyzing changes to tables less confusing and there would
>> be no need to clean useless untracked files when there are no table changes.
>> 
>> CC: [e-mail]
>> Signed-off-by: Ani Sinha <[e-mail]>
>> Message-Id: <[e-mail]>
>> Reviewed-by: Michael S. Tsirkin <[e-mail]>
>> Signed-off-by: Michael S. Tsirkin <[e-mail]>
>> ---
>> tests/qtest/bios-tables-test.c | 14 +++++++++++++-
>> 1 file changed, 13 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
>> index 71af5cf69f..fe6a9a8563 100644
>> --- a/tests/qtest/bios-tables-test.c
>> +++ b/tests/qtest/bios-tables-test.c
>> @@ -112,6 +112,7 @@ static const char *iasl;
>> #endif
>> 
>> static int verbosity_level;
>> +static GArray *load_expected_aml(test_data *data);
>> 
>> static bool compare_signature(const AcpiSdtTable *sdt, const char *signature)
>> {
>> @@ -244,21 +245,32 @@ static void test_acpi_fadt_table(test_data *data)
>> 
>> static void dump_aml_files(test_data *data, bool rebuild)
>> {
>> -    AcpiSdtTable *sdt;
>> +    AcpiSdtTable *sdt, *exp_sdt;
>>     GError *error = NULL;
>>     gchar *aml_file = NULL;
>> +    test_data exp_data = {};
>>     gint fd;
>>     ssize_t ret;
>>     int i;
>> 
>> +    exp_data.tables = load_expected_aml(data);
>>     for (i = 0; i < data->tables->len; ++i) {
>>         const char *ext = data->variant ? data->variant : "";
>>         sdt = &g_array_index(data->tables, AcpiSdtTable, i);
>> +        exp_sdt = &g_array_index(exp_data.tables, AcpiSdtTable, i);
>>         g_assert(sdt->aml);
>> +        g_assert(exp_sdt->aml);
>> 
>>         if (rebuild) {
>>             aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine,
>>                                        sdt->aml, ext);
>> +            if (!g_file_test(aml_file, G_FILE_TEST_EXISTS) &&
>> +                sdt->aml_len == exp_sdt->aml_len &&
>> +                !memcmp(sdt->aml, exp_sdt->aml, sdt->aml_len)) {
>> +                /* identical tables, no need to write new files */
>> +                g_free(aml_file);
>> +                continue;
>> +            }
>>             fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
>>                         S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
>>             if (fd < 0) {
>

Patch

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 71af5cf69f..fe6a9a8563 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -112,6 +112,7 @@  static const char *iasl;
 #endif
 
 static int verbosity_level;
+static GArray *load_expected_aml(test_data *data);
 
 static bool compare_signature(const AcpiSdtTable *sdt, const char *signature)
 {
@@ -244,21 +245,32 @@  static void test_acpi_fadt_table(test_data *data)
 
 static void dump_aml_files(test_data *data, bool rebuild)
 {
-    AcpiSdtTable *sdt;
+    AcpiSdtTable *sdt, *exp_sdt;
     GError *error = NULL;
     gchar *aml_file = NULL;
+    test_data exp_data = {};
     gint fd;
     ssize_t ret;
     int i;
 
+    exp_data.tables = load_expected_aml(data);
     for (i = 0; i < data->tables->len; ++i) {
         const char *ext = data->variant ? data->variant : "";
         sdt = &g_array_index(data->tables, AcpiSdtTable, i);
+        exp_sdt = &g_array_index(exp_data.tables, AcpiSdtTable, i);
         g_assert(sdt->aml);
+        g_assert(exp_sdt->aml);
 
         if (rebuild) {
             aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine,
                                        sdt->aml, ext);
+            if (!g_file_test(aml_file, G_FILE_TEST_EXISTS) &&
+                sdt->aml_len == exp_sdt->aml_len &&
+                !memcmp(sdt->aml, exp_sdt->aml, sdt->aml_len)) {
+                /* identical tables, no need to write new files */
+                g_free(aml_file);
+                continue;
+            }
             fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
                         S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
             if (fd < 0) {