Re: [Qemu-devel] [PATCH for-3.0 ?] tests/libqtest: Improve kill_qemu() assert

[Richard Henderson <richard.henderson@linaro.org>]

On 07/20/2018 08:49 AM, Peter Maydell wrote:
> On 20 July 2018 at 16:48, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 07/20/2018 12:39 PM, Peter Maydell wrote:
>>> In kill_qemu() we have an assert that checks that the QEMU process
>>> didn't dump core:
>>>             assert(!WCOREDUMP(wstatus));
>>>
>>> Unfortunately the WCOREDUMP macro here means the resulting message
>>> is not very easy to comprehend on at least some systems:
>>>
>>> ahci-test: tests/libqtest.c:113: kill_qemu: Assertion `!(((__extension__ (((union { __typeof(wstatus) __in; int __i; }) { .__in = (wstatus) }).__i))) & 0x80)' failed.
>>>
>>> and it doesn't identify what signal the process took.
>>>
>>> Instead of using a raw assert, print the information in an
>>> easier to understand way:
>>>
>>> /i386/ahci/sanity: libqtest.c: kill_qemu() tried to terminate QEMU process but it dumped core with signal 11
>>
>> Less cryptic, indeed.
>>
>>> ahci-test: tests/libqtest.c:118: kill_qemu: Assertion `0' failed.
>>
>> This file:line is not very relevant, ...
> 
>> ... what about directly using abort() here?
> 
> I did actually start with that, but decided I'd rather have
> the file-and-line in there to direct people more quickly
> to the immediate point where we asserted.

You already print the file, just include the line.  Perhaps

  fprintf(stderr,
          "%s:%d: kill_qemu tried to terminate QEMU "
          "process but it dumped core with signal %s\n",
          __FILE__, __LINE__, strsignal(WTERMSIG(wstatus)));
  abort();

Not that I expect the signal to ever be anything other than 11,
and that being one of the handful that are consistent across
pretty much all unix systems.  But still.


r~


PS: The bike shed should be blue.