| Message ID | 20220112130332.1648664-2-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | acpi: fix short OEM [Table] ID padding | expand |
On Wed, Jan 12, 2022 at 08:03:29AM -0500, Igor Mammedov wrote: > The next commit will revert OEM fields padding with whitespace to > padding with '\0' as it was before [1]. As result test_oem_fields() will > fail due to unexpectedly smaller ID sizes read from QEMU ACPI tables. > > Pad OEM_ID/OEM_TABLE_ID manually with spaces so that values the test > puts on QEMU CLI and expected values match. > > 1) 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed") > Signed-off-by: Igor Mammedov <imammedo@redhat.com> That's kind of ugly in that we do not test shorter names then. How about we pad with \0 instead? And add a comment explaining why it's done. > --- > tests/qtest/bios-tables-test.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > index e6b72d9026..90c9f6a0a2 100644 > --- a/tests/qtest/bios-tables-test.c > +++ b/tests/qtest/bios-tables-test.c > @@ -71,9 +71,10 @@ > > #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML" > > -#define OEM_ID "TEST" > -#define OEM_TABLE_ID "OEM" > -#define OEM_TEST_ARGS "-machine x-oem-id="OEM_ID",x-oem-table-id="OEM_TABLE_ID > +#define OEM_ID "TEST " > +#define OEM_TABLE_ID "OEM " > +#define OEM_TEST_ARGS "-machine x-oem-id='" OEM_ID "',x-oem-table-id='" \ > + OEM_TABLE_ID "'" > > typedef struct { > bool tcg_only; > @@ -1519,11 +1520,7 @@ static void test_acpi_q35_slic(void) > static void test_oem_fields(test_data *data) > { > int i; > - char oem_id[6]; > - char oem_table_id[8]; > > - strpadcpy(oem_id, sizeof oem_id, OEM_ID, ' '); > - strpadcpy(oem_table_id, sizeof oem_table_id, OEM_TABLE_ID, ' '); > for (i = 0; i < data->tables->len; ++i) { > AcpiSdtTable *sdt; > > @@ -1533,8 +1530,8 @@ static void test_oem_fields(test_data *data) > continue; > } > > - g_assert(memcmp(sdt->aml + 10, oem_id, 6) == 0); > - g_assert(memcmp(sdt->aml + 16, oem_table_id, 8) == 0); > + g_assert(memcmp(sdt->aml + 10, OEM_ID, 6) == 0); > + g_assert(memcmp(sdt->aml + 16, OEM_TABLE_ID, 8) == 0); > } > } > > -- > 2.31.1
On Wed, 12 Jan 2022 08:44:19 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Jan 12, 2022 at 08:03:29AM -0500, Igor Mammedov wrote: > > The next commit will revert OEM fields padding with whitespace to > > padding with '\0' as it was before [1]. As result test_oem_fields() will > > fail due to unexpectedly smaller ID sizes read from QEMU ACPI tables. > > > > Pad OEM_ID/OEM_TABLE_ID manually with spaces so that values the test > > puts on QEMU CLI and expected values match. > > > > 1) 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed") > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > That's kind of ugly in that we do not test > shorter names then. How about we pad with \0 instead? test_acpi_q35_slic() should cover short OEM_TABLE_ID. also padding in this patch makes test_oem_fields() cleaner and simplifies 3/4, switching to \0 here would require merging this patch with the fix itself to avoid breaking bisection. If you still prefer to have test_oem_fields() test short names, I can post following on top that can to it without breaking bisection: diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index 90c9f6a0a2..0fd7cf1f89 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -71,8 +71,8 @@ #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML" -#define OEM_ID "TEST " -#define OEM_TABLE_ID "OEM " +#define OEM_ID "TEST" +#define OEM_TABLE_ID "OEM" #define OEM_TEST_ARGS "-machine x-oem-id='" OEM_ID "',x-oem-table-id='" \ OEM_TABLE_ID "'" @@ -1530,8 +1530,8 @@ static void test_oem_fields(test_data *data) continue; } - g_assert(memcmp(sdt->aml + 10, OEM_ID, 6) == 0); - g_assert(memcmp(sdt->aml + 16, OEM_TABLE_ID, 8) == 0); + g_assert(strncmp((char *)sdt->aml + 10, OEM_ID, 6) == 0); + g_assert(strncmp((char *)sdt->aml + 16, OEM_TABLE_ID, 8) == 0); } } > And add a comment explaining why it's done. > > > --- > > tests/qtest/bios-tables-test.c | 15 ++++++--------- > > 1 file changed, 6 insertions(+), 9 deletions(-) > > > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > > index e6b72d9026..90c9f6a0a2 100644 > > --- a/tests/qtest/bios-tables-test.c > > +++ b/tests/qtest/bios-tables-test.c > > @@ -71,9 +71,10 @@ > > > > #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML" > > > > -#define OEM_ID "TEST" > > -#define OEM_TABLE_ID "OEM" > > -#define OEM_TEST_ARGS "-machine x-oem-id="OEM_ID",x-oem-table-id="OEM_TABLE_ID > > +#define OEM_ID "TEST " > > +#define OEM_TABLE_ID "OEM " > > +#define OEM_TEST_ARGS "-machine x-oem-id='" OEM_ID "',x-oem-table-id='" \ > > + OEM_TABLE_ID "'" > > > > typedef struct { > > bool tcg_only; > > @@ -1519,11 +1520,7 @@ static void test_acpi_q35_slic(void) > > static void test_oem_fields(test_data *data) > > { > > int i; > > - char oem_id[6]; > > - char oem_table_id[8]; > > > > - strpadcpy(oem_id, sizeof oem_id, OEM_ID, ' '); > > - strpadcpy(oem_table_id, sizeof oem_table_id, OEM_TABLE_ID, ' '); > > for (i = 0; i < data->tables->len; ++i) { > > AcpiSdtTable *sdt; > > > > @@ -1533,8 +1530,8 @@ static void test_oem_fields(test_data *data) > > continue; > > } > > > > - g_assert(memcmp(sdt->aml + 10, oem_id, 6) == 0); > > - g_assert(memcmp(sdt->aml + 16, oem_table_id, 8) == 0); > > + g_assert(memcmp(sdt->aml + 10, OEM_ID, 6) == 0); > > + g_assert(memcmp(sdt->aml + 16, OEM_TABLE_ID, 8) == 0); > > } > > } > > > > -- > > 2.31.1 >
On Fri, Jan 14, 2022 at 12:48:20PM +0100, Igor Mammedov wrote: > On Wed, 12 Jan 2022 08:44:19 -0500 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Jan 12, 2022 at 08:03:29AM -0500, Igor Mammedov wrote: > > > The next commit will revert OEM fields padding with whitespace to > > > padding with '\0' as it was before [1]. As result test_oem_fields() will > > > fail due to unexpectedly smaller ID sizes read from QEMU ACPI tables. > > > > > > Pad OEM_ID/OEM_TABLE_ID manually with spaces so that values the test > > > puts on QEMU CLI and expected values match. > > > > > > 1) 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed") > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > That's kind of ugly in that we do not test > > shorter names then. How about we pad with \0 instead? > > > test_acpi_q35_slic() should cover short OEM_TABLE_ID. > > also padding in this patch makes test_oem_fields() cleaner > and simplifies 3/4, switching to \0 here would require > merging this patch with the fix itself to avoid breaking > bisection. > > If you still prefer to have test_oem_fields() test short > names, I can post following on top that can to it without > breaking bisection: > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > index 90c9f6a0a2..0fd7cf1f89 100644 > --- a/tests/qtest/bios-tables-test.c > +++ b/tests/qtest/bios-tables-test.c > @@ -71,8 +71,8 @@ > > #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML" > > -#define OEM_ID "TEST " > -#define OEM_TABLE_ID "OEM " > +#define OEM_ID "TEST" > +#define OEM_TABLE_ID "OEM" > #define OEM_TEST_ARGS "-machine x-oem-id='" OEM_ID "',x-oem-table-id='" \ > OEM_TABLE_ID "'" Don't we want to revert ARGS change too then? > @@ -1530,8 +1530,8 @@ static void test_oem_fields(test_data *data) > continue; > } > > - g_assert(memcmp(sdt->aml + 10, OEM_ID, 6) == 0); > - g_assert(memcmp(sdt->aml + 16, OEM_TABLE_ID, 8) == 0); > + g_assert(strncmp((char *)sdt->aml + 10, OEM_ID, 6) == 0); > + g_assert(strncmp((char *)sdt->aml + 16, OEM_TABLE_ID, 8) == 0); > } > } > Looks much cleaner to me. OK as a patch on top. > > > And add a comment explaining why it's done. > > > > > --- > > > tests/qtest/bios-tables-test.c | 15 ++++++--------- > > > 1 file changed, 6 insertions(+), 9 deletions(-) > > > > > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > > > index e6b72d9026..90c9f6a0a2 100644 > > > --- a/tests/qtest/bios-tables-test.c > > > +++ b/tests/qtest/bios-tables-test.c > > > @@ -71,9 +71,10 @@ > > > > > > #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML" > > > > > > -#define OEM_ID "TEST" > > > -#define OEM_TABLE_ID "OEM" > > > -#define OEM_TEST_ARGS "-machine x-oem-id="OEM_ID",x-oem-table-id="OEM_TABLE_ID > > > +#define OEM_ID "TEST " > > > +#define OEM_TABLE_ID "OEM " > > > +#define OEM_TEST_ARGS "-machine x-oem-id='" OEM_ID "',x-oem-table-id='" \ > > > + OEM_TABLE_ID "'" > > > > > > typedef struct { > > > bool tcg_only; > > > @@ -1519,11 +1520,7 @@ static void test_acpi_q35_slic(void) > > > static void test_oem_fields(test_data *data) > > > { > > > int i; > > > - char oem_id[6]; > > > - char oem_table_id[8]; > > > > > > - strpadcpy(oem_id, sizeof oem_id, OEM_ID, ' '); > > > - strpadcpy(oem_table_id, sizeof oem_table_id, OEM_TABLE_ID, ' '); > > > for (i = 0; i < data->tables->len; ++i) { > > > AcpiSdtTable *sdt; > > > > > > @@ -1533,8 +1530,8 @@ static void test_oem_fields(test_data *data) > > > continue; > > > } > > > > > > - g_assert(memcmp(sdt->aml + 10, oem_id, 6) == 0); > > > - g_assert(memcmp(sdt->aml + 16, oem_table_id, 8) == 0); > > > + g_assert(memcmp(sdt->aml + 10, OEM_ID, 6) == 0); > > > + g_assert(memcmp(sdt->aml + 16, OEM_TABLE_ID, 8) == 0); > > > } > > > } > > > > > > -- > > > 2.31.1 > >
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index e6b72d9026..90c9f6a0a2 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -71,9 +71,10 @@ #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML" -#define OEM_ID "TEST" -#define OEM_TABLE_ID "OEM" -#define OEM_TEST_ARGS "-machine x-oem-id="OEM_ID",x-oem-table-id="OEM_TABLE_ID +#define OEM_ID "TEST " +#define OEM_TABLE_ID "OEM " +#define OEM_TEST_ARGS "-machine x-oem-id='" OEM_ID "',x-oem-table-id='" \ + OEM_TABLE_ID "'" typedef struct { bool tcg_only; @@ -1519,11 +1520,7 @@ static void test_acpi_q35_slic(void) static void test_oem_fields(test_data *data) { int i; - char oem_id[6]; - char oem_table_id[8]; - strpadcpy(oem_id, sizeof oem_id, OEM_ID, ' '); - strpadcpy(oem_table_id, sizeof oem_table_id, OEM_TABLE_ID, ' '); for (i = 0; i < data->tables->len; ++i) { AcpiSdtTable *sdt; @@ -1533,8 +1530,8 @@ static void test_oem_fields(test_data *data) continue; } - g_assert(memcmp(sdt->aml + 10, oem_id, 6) == 0); - g_assert(memcmp(sdt->aml + 16, oem_table_id, 8) == 0); + g_assert(memcmp(sdt->aml + 10, OEM_ID, 6) == 0); + g_assert(memcmp(sdt->aml + 16, OEM_TABLE_ID, 8) == 0); } }
The next commit will revert OEM fields padding with whitespace to padding with '\0' as it was before [1]. As result test_oem_fields() will fail due to unexpectedly smaller ID sizes read from QEMU ACPI tables. Pad OEM_ID/OEM_TABLE_ID manually with spaces so that values the test puts on QEMU CLI and expected values match. 1) 602b458201 ("acpi: Permit OEM ID and OEM table ID fields to be changed") Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- tests/qtest/bios-tables-test.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)