[PATCH] tests/acpi/bios-tables-test: do not write new blobs unless there are changes

[PATCH] tests/acpi/bios-tables-test: do not write new blobs unless there are changes

[Ani Sinha]

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. If there are no changes,
no new files are written for test variants. However, existing files continue
to be overwritten regardless of whether there are changes. Hence, new files
will be generated only when there are actual changes in the tables.
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: peter.maydell@linaro.org
Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 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 9f4bc15aab..743b509e93 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -109,6 +109,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)
 {
@@ -241,21 +242,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) {
-- 
2.42.0

Re: [PATCH] tests/acpi/bios-tables-test: do not write new blobs unless there are changes

[Igor Mammedov]

On Thu,  2 Nov 2023 13:46:24 +0530
Ani Sinha <anisinha@redhat.com> wrote:

> 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.

> If there are no changes, no new files are written for test variants.
> However, existing files continue to be overwritten regardless of whether there are changes.
> Hence, new files will be generated only when there are actual changes in the tables.

You lost me in those 3 sentences. Perhaps rephrasing and adding examples
wold make it readable. (aka what's (not)writen and when)


> 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.

what happens if an absolutely new table has been introduced which
is not mentioned in tests yet (will it be dumped or not)?

> 
> CC: peter.maydell@linaro.org
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
>  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 9f4bc15aab..743b509e93 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -109,6 +109,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)
>  {
> @@ -241,21 +242,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) {

Re: [PATCH] tests/acpi/bios-tables-test: do not write new blobs unless there are changes

[Ani Sinha]

> On 06-Nov-2023, at 7:45 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> On Thu,  2 Nov 2023 13:46:24 +0530
> Ani Sinha <anisinha@redhat.com> wrote:
> 
>> 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.
> 
>> If there are no changes, no new files are written for test variants.
>> However, existing files continue to be overwritten regardless of whether there are changes.
>> Hence, new files will be generated only when there are actual changes in the tables.
> 
> You lost me in those 3 sentences. Perhaps rephrasing and adding examples
> wold make it readable. (aka what's (not)writen and when)

OK I will try in v2.

> 
> 
>> 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.
> 
> what happens if an absolutely new table has been introduced which
> is not mentioned in tests yet (will it be dumped or not)?

When a new table is introduced, there is no existing data to compare the tables with. In this case, it will result in assertion in the following line:

>     g_assert(exp_sdt.aml_file);

I am not sure what to do in this case except to unconditionally dump all tables. Maybe introduce another flag? Not sure.

> 
>> 
>> CC: peter.maydell@linaro.org
>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>> ---
>> 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 9f4bc15aab..743b509e93 100644
>> --- a/tests/qtest/bios-tables-test.c
>> +++ b/tests/qtest/bios-tables-test.c
>> @@ -109,6 +109,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)
>> {
>> @@ -241,21 +242,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) {

Re: [PATCH] tests/acpi/bios-tables-test: do not write new blobs unless there are changes

[Ani Sinha]

> On 06-Nov-2023, at 8:43 PM, Ani Sinha <anisinha@redhat.com> wrote:
> 
> 
> 
>> On 06-Nov-2023, at 7:45 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> 
>> On Thu,  2 Nov 2023 13:46:24 +0530
>> Ani Sinha <anisinha@redhat.com> wrote:
>> 
>>> 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.
>> 
>>> If there are no changes, no new files are written for test variants.
>>> However, existing files continue to be overwritten regardless of whether there are changes.
>>> Hence, new files will be generated only when there are actual changes in the tables.
>> 
>> You lost me in those 3 sentences. Perhaps rephrasing and adding examples
>> wold make it readable. (aka what's (not)writen and when)
> 
> OK I will try in v2.
> 
>> 
>> 
>>> 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.
>> 
>> what happens if an absolutely new table has been introduced which
>> is not mentioned in tests yet (will it be dumped or not)?
> 
> When a new table is introduced, there is no existing data to compare the tables with. In this case, it will result in assertion in the following line:
> 
>>    g_assert(exp_sdt.aml_file);
> 
> I am not sure what to do in this case except to unconditionally dump all tables. Maybe introduce another flag? Not sure.

Oh wait, this patch works as is since we have said in bios-tables-test.c:

>  add empty files for new tables, if any, under tests/data/acpi  

So for new tables the lengths will not match and the table should be dumped. I need to test this case once. 

> 
>> 
>>> 
>>> CC: peter.maydell@linaro.org
>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>>> ---
>>> 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 9f4bc15aab..743b509e93 100644
>>> --- a/tests/qtest/bios-tables-test.c
>>> +++ b/tests/qtest/bios-tables-test.c
>>> @@ -109,6 +109,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)
>>> {
>>> @@ -241,21 +242,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) {