Re: [PATCH 1/4] tests: acpi: manually pad OEM_ID/OEM_TABLE_ID for test_oem_fields() test

[Michael S. Tsirkin]

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