| Message ID | 1395655353-19051-1-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 24.03.2014 11:02, schrieb Marcel Apfelbaum: > There is an issue with iasl on big endian machines: It > cannot disassemble acpi tables taken from little endian > machines, so we cannot check the expected tables. > > The acpi test will check if the expected aml files > can be disassembled, and will issue an warning not > failing the test on those machines until this > problem is solved by the acpica community. > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > --- > V3 -> V4: > Addressed all upstream comments: > - Instead of disabling iasl for big endian machines, > the test checks if the expected aml files can be > disassembled, if not it issues a warning instead > of failing the test > > V2 -> V3: > Addressed Michael S. Tsirkin's review: > - tests don't need to re-run detection, use configure > to figure out if it is an LE machine. > > V1 -> V2: > Addressed an offline tip for a much cleaner > macro line, thanks! > > tests/acpi-test.c | 26 +++++++++++++++----------- > 1 file changed, 15 insertions(+), 11 deletions(-) > > diff --git a/tests/acpi-test.c b/tests/acpi-test.c > index 249fe03..76fbccf 100644 > --- a/tests/acpi-test.c > +++ b/tests/acpi-test.c > @@ -456,13 +456,12 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt) > /* pass 'out' and 'out_err' in order to be redirected */ > ret = g_spawn_command_line_sync(command_line->str, &out, &out_err, NULL, &error); > g_assert_no_error(error); > - > 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); > + ret = (sdt->asl_len > 0); > } > > g_free(out); > @@ -560,15 +559,20 @@ static void test_acpi_asl(test_data *data) > g_assert(!err || exp_err); > > if (g_strcmp0(asl->str, exp_asl->str)) { > - uint32_t signature = cpu_to_le32(exp_sdt->header.signature); > - sdt->tmp_files_retain = true; > - exp_sdt->tmp_files_retain = true; > - fprintf(stderr, > - "acpi-test: Warning! %.4s mismatch. " > - "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n", > - (gchar *)&signature, > - sdt->asl_file, sdt->aml_file, > - exp_sdt->asl_file, exp_sdt->aml_file); > + if (exp_err) { > + fprintf(stderr, > + "Warning! iasl couldn't parse the expected aml\n"); Should these two be g_test_message()s? That's used in other tests - downside is the message appears in the .xml files generated as intermediary for the HTML report but did (in my case) not show up on stdout/stderr. Maybe we should try to fix that? Paolo? Stefan? Regards, Andreas > + } else { > + uint32_t signature = cpu_to_le32(exp_sdt->header.signature); > + sdt->tmp_files_retain = true; > + exp_sdt->tmp_files_retain = true; > + fprintf(stderr, > + "acpi-test: Warning! %.4s mismatch. " > + "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n", > + (gchar *)&signature, > + sdt->asl_file, sdt->aml_file, > + exp_sdt->asl_file, exp_sdt->aml_file); > + } > } > g_string_free(asl, true); > g_string_free(exp_asl, true); >
On Mon, 2014-03-24 at 11:33 +0100, Andreas Färber wrote: > Am 24.03.2014 11:02, schrieb Marcel Apfelbaum: > > There is an issue with iasl on big endian machines: It > > cannot disassemble acpi tables taken from little endian > > machines, so we cannot check the expected tables. > > > > The acpi test will check if the expected aml files > > can be disassembled, and will issue an warning not > > failing the test on those machines until this > > problem is solved by the acpica community. > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > --- > > V3 -> V4: > > Addressed all upstream comments: > > - Instead of disabling iasl for big endian machines, > > the test checks if the expected aml files can be > > disassembled, if not it issues a warning instead > > of failing the test > > > > V2 -> V3: > > Addressed Michael S. Tsirkin's review: > > - tests don't need to re-run detection, use configure > > to figure out if it is an LE machine. > > > > V1 -> V2: > > Addressed an offline tip for a much cleaner > > macro line, thanks! > > > > tests/acpi-test.c | 26 +++++++++++++++----------- > > 1 file changed, 15 insertions(+), 11 deletions(-) > > > > diff --git a/tests/acpi-test.c b/tests/acpi-test.c > > index 249fe03..76fbccf 100644 > > --- a/tests/acpi-test.c > > +++ b/tests/acpi-test.c > > @@ -456,13 +456,12 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt) > > /* pass 'out' and 'out_err' in order to be redirected */ > > ret = g_spawn_command_line_sync(command_line->str, &out, &out_err, NULL, &error); > > g_assert_no_error(error); > > - > > 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); > > + ret = (sdt->asl_len > 0); > > } > > > > g_free(out); > > @@ -560,15 +559,20 @@ static void test_acpi_asl(test_data *data) > > g_assert(!err || exp_err); > > > > if (g_strcmp0(asl->str, exp_asl->str)) { > > - uint32_t signature = cpu_to_le32(exp_sdt->header.signature); > > - sdt->tmp_files_retain = true; > > - exp_sdt->tmp_files_retain = true; > > - fprintf(stderr, > > - "acpi-test: Warning! %.4s mismatch. " > > - "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n", > > - (gchar *)&signature, > > - sdt->asl_file, sdt->aml_file, > > - exp_sdt->asl_file, exp_sdt->aml_file); > > + if (exp_err) { > > + fprintf(stderr, > > + "Warning! iasl couldn't parse the expected aml\n"); > > Should these two be g_test_message()s? That's used in other tests - > downside is the message appears in the .xml files generated as > intermediary for the HTML report but did (in my case) not show up on > stdout/stderr. Maybe we should try to fix that? Paolo? Stefan? I also don't see them in stdout/stderr. This is the *only* reason I use fprintf. Thanks, Marcel > > Regards, > Andreas > > > + } else { > > + uint32_t signature = cpu_to_le32(exp_sdt->header.signature); > > + sdt->tmp_files_retain = true; > > + exp_sdt->tmp_files_retain = true; > > + fprintf(stderr, > > + "acpi-test: Warning! %.4s mismatch. " > > + "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n", > > + (gchar *)&signature, > > + sdt->asl_file, sdt->aml_file, > > + exp_sdt->asl_file, exp_sdt->aml_file); > > + } > > } > > g_string_free(asl, true); > > g_string_free(exp_asl, true); > > > >
Am 24.03.2014 11:42, schrieb Michael S. Tsirkin: > On Mon, Mar 24, 2014 at 12:02:33PM +0200, Marcel Apfelbaum wrote: >> There is an issue with iasl on big endian machines: It >> cannot disassemble acpi tables taken from little endian >> machines, so we cannot check the expected tables. >> >> The acpi test will check if the expected aml files >> can be disassembled, and will issue an warning not >> failing the test on those machines until this >> problem is solved by the acpica community. >> >> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > Applied, improvements such as these suggested by Andreas > can be incremental on top. Agreed. Andreas
On Mon, Mar 24, 2014 at 02:17:10PM +0200, Marcel Apfelbaum wrote: > On Mon, 2014-03-24 at 11:33 +0100, Andreas Färber wrote: > > Am 24.03.2014 11:02, schrieb Marcel Apfelbaum: > > > There is an issue with iasl on big endian machines: It > > > cannot disassemble acpi tables taken from little endian > > > machines, so we cannot check the expected tables. > > > > > > The acpi test will check if the expected aml files > > > can be disassembled, and will issue an warning not > > > failing the test on those machines until this > > > problem is solved by the acpica community. > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > --- > > > V3 -> V4: > > > Addressed all upstream comments: > > > - Instead of disabling iasl for big endian machines, > > > the test checks if the expected aml files can be > > > disassembled, if not it issues a warning instead > > > of failing the test > > > > > > V2 -> V3: > > > Addressed Michael S. Tsirkin's review: > > > - tests don't need to re-run detection, use configure > > > to figure out if it is an LE machine. > > > > > > V1 -> V2: > > > Addressed an offline tip for a much cleaner > > > macro line, thanks! > > > > > > tests/acpi-test.c | 26 +++++++++++++++----------- > > > 1 file changed, 15 insertions(+), 11 deletions(-) > > > > > > diff --git a/tests/acpi-test.c b/tests/acpi-test.c > > > index 249fe03..76fbccf 100644 > > > --- a/tests/acpi-test.c > > > +++ b/tests/acpi-test.c > > > @@ -456,13 +456,12 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt) > > > /* pass 'out' and 'out_err' in order to be redirected */ > > > ret = g_spawn_command_line_sync(command_line->str, &out, &out_err, NULL, &error); > > > g_assert_no_error(error); > > > - > > > 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); > > > + ret = (sdt->asl_len > 0); > > > } > > > > > > g_free(out); > > > @@ -560,15 +559,20 @@ static void test_acpi_asl(test_data *data) > > > g_assert(!err || exp_err); > > > > > > if (g_strcmp0(asl->str, exp_asl->str)) { > > > - uint32_t signature = cpu_to_le32(exp_sdt->header.signature); > > > - sdt->tmp_files_retain = true; > > > - exp_sdt->tmp_files_retain = true; > > > - fprintf(stderr, > > > - "acpi-test: Warning! %.4s mismatch. " > > > - "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n", > > > - (gchar *)&signature, > > > - sdt->asl_file, sdt->aml_file, > > > - exp_sdt->asl_file, exp_sdt->aml_file); > > > + if (exp_err) { > > > + fprintf(stderr, > > > + "Warning! iasl couldn't parse the expected aml\n"); > > > > Should these two be g_test_message()s? That's used in other tests - > > downside is the message appears in the .xml files generated as > > intermediary for the HTML report but did (in my case) not show up on > > stdout/stderr. Maybe we should try to fix that? Paolo? Stefan? > I also don't see them in stdout/stderr. This is the *only* > reason I use fprintf. Did you try running gtester without -q or even with -v? (You can edit tests/Makefile to do that.) Stefan
On 24 March 2014 13:23, Stefan Hajnoczi <stefanha@redhat.com> wrote: > Did you try running gtester without -q or even with -v? (You can edit > tests/Makefile to do that.) ...or use make V=1. thanks -- PMM
On Mon, 2014-03-24 at 13:24 +0000, Peter Maydell wrote: > On 24 March 2014 13:23, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > Did you try running gtester without -q or even with -v? (You can edit > > tests/Makefile to do that.) > > ...or use make V=1. I'll try both, but do we really want to run make check with extra flags in order to see g_test_message()s ? Thanks, Marcel > > thanks > -- PMM
On Mon, 2014-03-24 at 13:24 +0000, Peter Maydell wrote: > On 24 March 2014 13:23, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > Did you try running gtester without -q or even with -v? (You can edit > > tests/Makefile to do that.) > > ...or use make V=1. I tried the verbose mode, no g_test_message output. Thanks, Marcel > > thanks > -- PMM >
On Mon, 2014-03-24 at 15:43 +0200, Marcel Apfelbaum wrote: > On Mon, 2014-03-24 at 13:24 +0000, Peter Maydell wrote: > > On 24 March 2014 13:23, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > Did you try running gtester without -q or even with -v? (You can edit > > > tests/Makefile to do that.) > > > > ...or use make V=1. > I tried the verbose mode, no g_test_message output. But g_message can be seen on stdout, is this a valid option? Thanks, Marcel > > Thanks, > Marcel > > > > > thanks > > -- PMM > > > > > >
Am 24.03.2014 14:24, schrieb Peter Maydell: > On 24 March 2014 13:23, Stefan Hajnoczi <stefanha@redhat.com> wrote: >> Did you try running gtester without -q or even with -v? (You can edit >> tests/Makefile to do that.) > > ...or use make V=1. V=1 I already tried. (And I also suggested to use that for Travis since otherwise errors are really hard to identify.) I also recall a patch from Stefan tweaking gtester options that I've not (yet) picked up...although that just seemed to make V=1 behavior the default IIRC? Regards, Andreas
diff --git a/tests/acpi-test.c b/tests/acpi-test.c index 249fe03..76fbccf 100644 --- a/tests/acpi-test.c +++ b/tests/acpi-test.c @@ -456,13 +456,12 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt) /* pass 'out' and 'out_err' in order to be redirected */ ret = g_spawn_command_line_sync(command_line->str, &out, &out_err, NULL, &error); g_assert_no_error(error); - 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); + ret = (sdt->asl_len > 0); } g_free(out); @@ -560,15 +559,20 @@ static void test_acpi_asl(test_data *data) g_assert(!err || exp_err); if (g_strcmp0(asl->str, exp_asl->str)) { - uint32_t signature = cpu_to_le32(exp_sdt->header.signature); - sdt->tmp_files_retain = true; - exp_sdt->tmp_files_retain = true; - fprintf(stderr, - "acpi-test: Warning! %.4s mismatch. " - "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n", - (gchar *)&signature, - sdt->asl_file, sdt->aml_file, - exp_sdt->asl_file, exp_sdt->aml_file); + if (exp_err) { + fprintf(stderr, + "Warning! iasl couldn't parse the expected aml\n"); + } else { + uint32_t signature = cpu_to_le32(exp_sdt->header.signature); + sdt->tmp_files_retain = true; + exp_sdt->tmp_files_retain = true; + fprintf(stderr, + "acpi-test: Warning! %.4s mismatch. " + "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n", + (gchar *)&signature, + sdt->asl_file, sdt->aml_file, + exp_sdt->asl_file, exp_sdt->aml_file); + } } g_string_free(asl, true); g_string_free(exp_asl, true);
There is an issue with iasl on big endian machines: It cannot disassemble acpi tables taken from little endian machines, so we cannot check the expected tables. The acpi test will check if the expected aml files can be disassembled, and will issue an warning not failing the test on those machines until this problem is solved by the acpica community. Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> --- V3 -> V4: Addressed all upstream comments: - Instead of disabling iasl for big endian machines, the test checks if the expected aml files can be disassembled, if not it issues a warning instead of failing the test V2 -> V3: Addressed Michael S. Tsirkin's review: - tests don't need to re-run detection, use configure to figure out if it is an LE machine. V1 -> V2: Addressed an offline tip for a much cleaner macro line, thanks! tests/acpi-test.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)