From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59010) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WIgrG-0004IZ-Sj for qemu-devel@nongnu.org; Wed, 26 Feb 2014 10:57:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WIgrA-0002X9-Ry for qemu-devel@nongnu.org; Wed, 26 Feb 2014 10:57:18 -0500 Received: from mx1.redhat.com ([209.132.183.28]:10280) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WIgrA-0002Wr-7p for qemu-devel@nongnu.org; Wed, 26 Feb 2014 10:57:12 -0500 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s1QFvARO029972 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 26 Feb 2014 10:57:11 -0500 Message-ID: <1393430237.31381.19.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Wed, 26 Feb 2014 17:57:17 +0200 In-Reply-To: <20140226155259.GF15330@redhat.com> References: <1393243758-31946-1-git-send-email-marcel.a@redhat.com> <1393243758-31946-3-git-send-email-marcel.a@redhat.com> <20140226152538.GA15330@redhat.com> <1393429179.31381.16.camel@localhost.localdomain> <20140226155259.GF15330@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/2] acpi-test: issue errors instead of warnings when possible List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org 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 > > > > > > 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 > > > >