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

[Ani Sinha <anisinha@redhat.com>]

> 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) {