Re: [Qemu-devel] [PATCH] acpi unit-test: resolved iasl crash

[Marcel Apfelbaum]

On Mon, 2014-01-06 at 13:43 +0200, Michael S. Tsirkin wrote:
> On Sun, Dec 29, 2013 at 02:32:50PM +0200, Marcel Apfelbaum wrote:
> > It seems that iasl has an issue when disassembles
> > some ACPI tables using the command line:
> > iasl -e DSDT -e SSDT -d HPET
> > I opened a bug on iasl project:
> > https://github.com/acpica/acpica/issues/20
> > 
> > Modified the iasl command line to "iasl -d HPET"
> > until the problem is solved. The command line
> > remained the same for DSDT and SSDT tables.
> > 
> > Reported-by:Michael S. Tsirkin <address@hidden>
> 
> space needed after :
Sure, missed that
> 
> > Signed-off-by: Marcel Apfelbaum <address@hidden>
> > ---
> >  tests/acpi-test.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tests/acpi-test.c b/tests/acpi-test.c
> > index 2d32b69..26d2e26 100644
> > --- a/tests/acpi-test.c
> > +++ b/tests/acpi-test.c
> > @@ -403,7 +403,7 @@ static void dump_aml_files(test_data *data, bool 
> > rebuild)
> >      }
> >  }
> >  
> > -static void load_asl(GArray *sdts, AcpiSdtTable *sdt)
> > +static void load_asl(GArray *sdts, AcpiSdtTable *sdt, bool referenceDsdt)
> >  {
> >      AcpiSdtTable *temp;
> >      GError *error = NULL;
> > @@ -419,9 +419,11 @@ static void load_asl(GArray *sdts, AcpiSdtTable *sdt)
> >  
> >      /* build command line */
> >      g_string_append_printf(command_line, " -p %s ", sdt->asl_file);
> > -    for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */
> > -        temp = &g_array_index(sdts, AcpiSdtTable, i);
> > -        g_string_append_printf(command_line, "-e %s ", temp->aml_file);
> > +    if (referenceDsdt) {
> > +        for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */
> > +            temp = &g_array_index(sdts, AcpiSdtTable, i);
> > +            g_string_append_printf(command_line, "-e %s ", temp->aml_file);
> > +        }
> >      }
> >      g_string_append_printf(command_line, "-d %s", sdt->aml_file);
> >  
> > @@ -510,14 +512,14 @@ static void test_acpi_asl(test_data *data)
> >      dump_aml_files(data, false);
> >      for (i = 0; i < data->ssdt_tables->len; ++i) {
> >          GString *asl, *exp_asl;
> > -
> > +        bool referenceDsdt = (i < 2); /* only for DSDT and SSDT */
> 
> 
> Please don't use camelCase for variables.
Sure, no idea why I did that in the first place...

> Can we check the table signature instead of looking at the index?
It is possible, but not really necessary because:
 1. The array of expected tables it is created as a shadow of the
    original tables. (We load the expected tables in the exact order as the 
originals)
 2. The loading is done based on table signatures.
So going over the index in *this* case it means going over the signatures.

Thanks for the review,
Marcel 

> 
> >          sdt = &g_array_index(data->ssdt_tables, AcpiSdtTable, i);
> >          exp_sdt = &g_array_index(exp_data.ssdt_tables, AcpiSdtTable, i);
> >  
> > -        load_asl(data->ssdt_tables, sdt);
> > +        load_asl(data->ssdt_tables, sdt, referenceDsdt);
> >          asl = normalize_asl(sdt->asl);
> >  
> > -        load_asl(exp_data.ssdt_tables, exp_sdt);
> > +        load_asl(exp_data.ssdt_tables, exp_sdt, referenceDsdt);
> >          exp_asl = normalize_asl(exp_sdt->asl);
> >  
> >          g_assert(!g_strcmp0(asl->str, exp_asl->str));
> > -- 
> > 1.8.3.1