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 <address@hidden>
>> ---
>>   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