[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 03/15] tests/acpi/bios-tables-test: do not write new blobs unl
From: |
Ani Sinha |
Subject: |
Re: [PULL 03/15] tests/acpi/bios-tables-test: do not write new blobs unless there are changes |
Date: |
Wed, 24 Jan 2024 15:17:22 +0530 |
> On 24-Jan-2024, at 14:28, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Fri, 1 Dec 2023 12:15:16 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> From: Ani Sinha <anisinha@redhat.com>
>>
>> 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: peter.maydell@linaro.org
>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>> Message-Id: <20231107044952.5461-1-anisinha@redhat.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@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 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) {
>