| Message ID | 1393243758-31946-3-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > --- > 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
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
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? > > > > > > > > --- > > > 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 > >
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 > > > >
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);
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> --- tests/acpi-test.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)