Re: [Qemu-devel] [PATCH v2 2/2] acpi-test: issue errors instead of warnings when possible

[Marcel Apfelbaum <marcel.a@redhat.com>]

On Wed, 2014-02-26 at 17:25 +0200, Michael S. Tsirkin wrote:
> On Mon, Feb 24, 2014 at 02:09:18PM +0200, Marcel Apfelbaum wrote:
> > If the expected (offline) acpi tables loaded correctly,
> > it is safe to assume the iasl installation is OK and
> > issue an error if the actual tables differ from expected
> > ones.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> 
> I'm not sure I agree with this one.
> It turned out to be too aggressive in the past
> as expected files get out of sync sometimes.
> What I would do is this:
> 	if IASL did not produce errors or warnings
> 	on expected files, it should not produce
> 	them on actual files either.
The whole point of this patch (series) was to find
a way to discover "real" errors in some cases (not in all).
What you suggested above removes the only "discoverable error"
and returns us to warning only mode.

However, the code does not look into stdout/stderr for iasl
errors (it would involve parsing because iasl *always* writes
to both), but checks if the invocation of iasl command terminates successfully,
merely saying: "Well, iasl invocation succeeded  for the expected files, there
is no reason to fail for the actual ones". The actual output is not verified.

No impact on "out of sync expected files".

Thanks,
Marcel

 

> 
> 
> > ---
> >  tests/acpi-test.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tests/acpi-test.c b/tests/acpi-test.c
> > index 2ce8c18..639b3ab 100644
> > --- a/tests/acpi-test.c
> > +++ b/tests/acpi-test.c
> > @@ -411,7 +411,7 @@ static bool compare_signature(AcpiSdtTable *sdt, uint32_t signature)
> >     return sdt->header.signature == signature;
> >  }
> >  
> > -static void load_asl(GArray *sdts, AcpiSdtTable *sdt)
> > +static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
> >  {
> >      AcpiSdtTable *temp;
> >      GError *error = NULL;
> > @@ -440,18 +440,22 @@ static void load_asl(GArray *sdts, AcpiSdtTable *sdt)
> >      g_string_append_printf(command_line, "-d %s", sdt->aml_file);
> >  
> >      /* pass 'out' and 'out_err' in order to be redirected */
> > -    g_spawn_command_line_sync(command_line->str, &out, &out_err, NULL, &error);
> > +    ret = g_spawn_command_line_sync(command_line->str, &out, &out_err, NULL, &error);
> >      g_assert_no_error(error);
> >  
> > -    ret = g_file_get_contents(sdt->asl_file, (gchar **)&sdt->asl,
> > -                              &sdt->asl_len, &error);
> > -    g_assert(ret);
> > -    g_assert_no_error(error);
> > -    g_assert(sdt->asl_len);
> > +    if (ret) {
> > +        ret = g_file_get_contents(sdt->asl_file, (gchar **)&sdt->asl,
> > +                                  &sdt->asl_len, &error);
> > +        g_assert(ret);
> > +        g_assert_no_error(error);
> > +        g_assert(sdt->asl_len);
> > +    }
> >  
> >      g_free(out);
> >      g_free(out_err);
> >      g_string_free(command_line, true);
> > +
> > +    return !ret;
> >  }
> >  
> >  #define COMMENT_END "*/"
> > @@ -518,6 +522,7 @@ static void test_acpi_asl(test_data *data)
> >      int i;
> >      AcpiSdtTable *sdt, *exp_sdt;
> >      test_data exp_data;
> > +    gboolean err;
> >  
> >      memset(&exp_data, 0, sizeof(exp_data));
> >      exp_data.tables = load_expected_aml(data);
> > @@ -531,7 +536,7 @@ static void test_acpi_asl(test_data *data)
> >          load_asl(data->tables, sdt);
> >          asl = normalize_asl(sdt->asl);
> >  
> > -        load_asl(exp_data.tables, exp_sdt);
> > +        err = load_asl(exp_data.tables, exp_sdt);
> >          exp_asl = normalize_asl(exp_sdt->asl);
> >  
> >          if (g_strcmp0(asl->str, exp_asl->str)) {
> > @@ -543,6 +548,9 @@ static void test_acpi_asl(test_data *data)
> >                      (gchar *)&exp_sdt->header.signature,
> >                      sdt->asl_file, sdt->aml_file,
> >                      exp_sdt->asl_file, exp_sdt->aml_file);
> > +            if (!err) { /* expected data loaded, iasl OK */
> > +                g_assert(false);
> > +            }
> >          }
> >          g_string_free(asl, true);
> >          g_string_free(exp_asl, true);
> > -- 
> > 1.8.3.1