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:52 +0200, Michael S. Tsirkin wrote:
> On Wed, Feb 26, 2014 at 05:39:39PM +0200, Marcel Apfelbaum wrote:
> > 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
> > 
> 
> aha
> what confused me is this:
>             if (!err) { /* expected data loaded, iasl OK */
>                g_assert(false);
>             }
> 
> how about
> 
> 	g_assert(err);
> 
> instead?
You are right!
I'll send a new version with this fix.

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