[Qemu-devel] [PATCH] tests/boot-serial: Do not delete the output file in case of errors

[Qemu-devel] [PATCH] tests/boot-serial: Do not delete the output file in case of errors

[Thomas Huth]

Peter reported that the boot-serial tester sometimes runs into timeouts
with SPARC guests. It's currently completely unclear whether this is due
to too much load on the host machine (so that the guest really just ran
too slow), or whether there is something wrong with the guest's firmware
boot. For further debugging, we need the serial output of the guest in
case of errors, so instead of unlinking the file immediately, this is
now only done in case of success. In case of error, print the name of the
file with the serial output via g_error() (which then also calls abort()
internally to mark the test as failed).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/boot-serial-test.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
index 4d6815c..952a2e7 100644
--- a/tests/boot-serial-test.c
+++ b/tests/boot-serial-test.c
@@ -111,9 +111,8 @@ static testdef_t tests[] = {
     { NULL }
 };
 
-static void check_guest_output(const testdef_t *test, int fd)
+static bool check_guest_output(const testdef_t *test, int fd)
 {
-    bool output_ok = false;
     int i, nbr = 0, pos = 0, ccnt;
     char ch;
 
@@ -125,8 +124,7 @@ static void check_guest_output(const testdef_t *test, int fd)
                 pos += 1;
                 if (test->expect[pos] == '\0') {
                     /* We've reached the end of the expected string! */
-                    output_ok = true;
-                    goto done;
+                    return true;
                 }
             } else {
                 pos = 0;
@@ -136,8 +134,7 @@ static void check_guest_output(const testdef_t *test, int fd)
         g_usleep(10000);
     }
 
-done:
-    g_assert(output_ok);
+    return false;
 }
 
 static void test_machine(const void *data)
@@ -180,12 +177,16 @@ static void test_machine(const void *data)
                                 "-no-shutdown -serial chardev:serial0 %s",
                                 codeparam, code ? codetmp : "",
                                 test->machine, serialtmp, test->extra);
-    unlink(serialtmp);
     if (code) {
         unlink(codetmp);
     }
 
-    check_guest_output(test, ser_fd);
+    if (!check_guest_output(test, ser_fd)) {
+        g_error("Failed to find expected string. Please check '%s'",
+                serialtmp);
+    }
+    unlink(serialtmp);
+
     qtest_quit(global_qtest);
 
     close(ser_fd);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] tests/boot-serial: Do not delete the output file in case of errors

[Mark Cave-Ayland]

On 22/05/18 09:30, Thomas Huth wrote:

> Peter reported that the boot-serial tester sometimes runs into timeouts
> with SPARC guests. It's currently completely unclear whether this is due
> to too much load on the host machine (so that the guest really just ran
> too slow), or whether there is something wrong with the guest's firmware
> boot. For further debugging, we need the serial output of the guest in
> case of errors, so instead of unlinking the file immediately, this is
> now only done in case of success. In case of error, print the name of the
> file with the serial output via g_error() (which then also calls abort()
> internally to mark the test as failed).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/boot-serial-test.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
> index 4d6815c..952a2e7 100644
> --- a/tests/boot-serial-test.c
> +++ b/tests/boot-serial-test.c
> @@ -111,9 +111,8 @@ static testdef_t tests[] = {
>       { NULL }
>   };
>   
> -static void check_guest_output(const testdef_t *test, int fd)
> +static bool check_guest_output(const testdef_t *test, int fd)
>   {
> -    bool output_ok = false;
>       int i, nbr = 0, pos = 0, ccnt;
>       char ch;
>   
> @@ -125,8 +124,7 @@ static void check_guest_output(const testdef_t *test, int fd)
>                   pos += 1;
>                   if (test->expect[pos] == '\0') {
>                       /* We've reached the end of the expected string! */
> -                    output_ok = true;
> -                    goto done;
> +                    return true;
>                   }
>               } else {
>                   pos = 0;
> @@ -136,8 +134,7 @@ static void check_guest_output(const testdef_t *test, int fd)
>           g_usleep(10000);
>       }
>   
> -done:
> -    g_assert(output_ok);
> +    return false;
>   }
>   
>   static void test_machine(const void *data)
> @@ -180,12 +177,16 @@ static void test_machine(const void *data)
>                                   "-no-shutdown -serial chardev:serial0 %s",
>                                   codeparam, code ? codetmp : "",
>                                   test->machine, serialtmp, test->extra);
> -    unlink(serialtmp);
>       if (code) {
>           unlink(codetmp);
>       }
>   
> -    check_guest_output(test, ser_fd);
> +    if (!check_guest_output(test, ser_fd)) {
> +        g_error("Failed to find expected string. Please check '%s'",
> +                serialtmp);
> +    }
> +    unlink(serialtmp);
> +
>       qtest_quit(global_qtest);
>   
>       close(ser_fd);

Is this for qemu-system-sparc rather than qemu-system-sparc64? The 
reason for asking is that OpenBIOS for SPARC32 writes zeros to all 
physical RAM before launching the Forth machine to work around a bug in 
older Solaris versions whereby the kernel page tables weren't explicitly 
zeroed and so the kernel would panic on boot due to junk PTEs.

At some point I should look at trying to move this to a higher level 
rather than doing this quite so early, but last time I looked at it IIRC 
there were complications related to the MMU mappings used at startup.


ATB,

Mark.

Re: [Qemu-devel] [PATCH] tests/boot-serial: Do not delete the output file in case of errors

[Thomas Huth]

On 22.05.2018 23:21, Mark Cave-Ayland wrote:
> On 22/05/18 09:30, Thomas Huth wrote:
> 
>> Peter reported that the boot-serial tester sometimes runs into timeouts
>> with SPARC guests. It's currently completely unclear whether this is due
>> to too much load on the host machine (so that the guest really just ran
>> too slow), or whether there is something wrong with the guest's firmware
>> boot. For further debugging, we need the serial output of the guest in
>> case of errors, so instead of unlinking the file immediately, this is
>> now only done in case of success. In case of error, print the name of the
>> file with the serial output via g_error() (which then also calls abort()
>> internally to mark the test as failed).
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   tests/boot-serial-test.c | 17 +++++++++--------
>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
>> index 4d6815c..952a2e7 100644
>> --- a/tests/boot-serial-test.c
>> +++ b/tests/boot-serial-test.c
>> @@ -111,9 +111,8 @@ static testdef_t tests[] = {
>>       { NULL }
>>   };
>>   -static void check_guest_output(const testdef_t *test, int fd)
>> +static bool check_guest_output(const testdef_t *test, int fd)
>>   {
>> -    bool output_ok = false;
>>       int i, nbr = 0, pos = 0, ccnt;
>>       char ch;
>>   @@ -125,8 +124,7 @@ static void check_guest_output(const testdef_t
>> *test, int fd)
>>                   pos += 1;
>>                   if (test->expect[pos] == '\0') {
>>                       /* We've reached the end of the expected string! */
>> -                    output_ok = true;
>> -                    goto done;
>> +                    return true;
>>                   }
>>               } else {
>>                   pos = 0;
>> @@ -136,8 +134,7 @@ static void check_guest_output(const testdef_t
>> *test, int fd)
>>           g_usleep(10000);
>>       }
>>   -done:
>> -    g_assert(output_ok);
>> +    return false;
>>   }
>>     static void test_machine(const void *data)
>> @@ -180,12 +177,16 @@ static void test_machine(const void *data)
>>                                   "-no-shutdown -serial
>> chardev:serial0 %s",
>>                                   codeparam, code ? codetmp : "",
>>                                   test->machine, serialtmp, test->extra);
>> -    unlink(serialtmp);
>>       if (code) {
>>           unlink(codetmp);
>>       }
>>   -    check_guest_output(test, ser_fd);
>> +    if (!check_guest_output(test, ser_fd)) {
>> +        g_error("Failed to find expected string. Please check '%s'",
>> +                serialtmp);
>> +    }
>> +    unlink(serialtmp);
>> +
>>       qtest_quit(global_qtest);
>>         close(ser_fd);
> 
> Is this for qemu-system-sparc rather than qemu-system-sparc64? The
> reason for asking is that OpenBIOS for SPARC32 writes zeros to all
> physical RAM before launching the Forth machine to work around a bug in
> older Solaris versions whereby the kernel page tables weren't explicitly
> zeroed and so the kernel would panic on boot due to junk PTEs.

Peter apparently hit the issue on both, SPARC32 and SPARC64, see:

https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg01057.html

and

https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg04553.html

... so no clue whether they are really related, two completely
independent issues, or a generic issue that we've just seen by accident
on Sparc machines so far. I hope we'll get a little bit more information
once somebody hits the issue with the above patch included.

 Thomas

Re: [Qemu-devel] [PATCH] tests/boot-serial: Do not delete the output file in case of errors

[Thomas Huth]

On 22.05.2018 10:30, Thomas Huth wrote:
> Peter reported that the boot-serial tester sometimes runs into timeouts
> with SPARC guests. It's currently completely unclear whether this is due
> to too much load on the host machine (so that the guest really just ran
> too slow), or whether there is something wrong with the guest's firmware
> boot. For further debugging, we need the serial output of the guest in
> case of errors, so instead of unlinking the file immediately, this is
> now only done in case of success. In case of error, print the name of the
> file with the serial output via g_error() (which then also calls abort()
> internally to mark the test as failed).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/boot-serial-test.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)

Ping!

Paolo, could you take this patch through your misc / character devices
tree, please?

 Thanks,
  Thomas

Re: [Qemu-devel] [PATCH] tests/boot-serial: Do not delete the output file in case of errors

[Paolo Bonzini]

On 27/06/2018 12:16, Thomas Huth wrote:
> On 22.05.2018 10:30, Thomas Huth wrote:
>> Peter reported that the boot-serial tester sometimes runs into timeouts
>> with SPARC guests. It's currently completely unclear whether this is due
>> to too much load on the host machine (so that the guest really just ran
>> too slow), or whether there is something wrong with the guest's firmware
>> boot. For further debugging, we need the serial output of the guest in
>> case of errors, so instead of unlinking the file immediately, this is
>> now only done in case of success. In case of error, print the name of the
>> file with the serial output via g_error() (which then also calls abort()
>> internally to mark the test as failed).
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  tests/boot-serial-test.c | 17 +++++++++--------
>>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> Ping!
> 
> Paolo, could you take this patch through your misc / character devices
> tree, please?
> 
>  Thanks,
>   Thomas
> 

Done, thanks.

Paolo