Re: [Qemu-devel] [PATCH v4 2/3] libqtest: fail if child coredumps

[Thomas Huth]

On 25.05.2018 14:15, Michael S. Tsirkin wrote:
> On Fri, May 25, 2018 at 08:10:48AM +0200, Thomas Huth wrote:
>> On 24.05.2018 20:25, Michael S. Tsirkin wrote:
>>> Right now tests report OK status if QEMU crashes during cleanup.
>>> Let's catch that case and fail the test.
>>>
>>> Signed-off-by: Michael S. Tsirkin <address@hidden>
>>> ---
>>>  tests/libqtest.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>>> index 43fb97e..f869854 100644
>>> --- a/tests/libqtest.c
>>> +++ b/tests/libqtest.c
>>> @@ -103,8 +103,15 @@ static int socket_accept(int sock)
>>>  static void kill_qemu(QTestState *s)
>>>  {
>>>      if (s->qemu_pid != -1) {
>>> +        int wstatus = 0;
>>> +        pid_t pid;
>>> +
>>>          kill(s->qemu_pid, SIGTERM);
>>> -        waitpid(s->qemu_pid, NULL, 0);
>>> +        pid = waitpid(s->qemu_pid, &wstatus, 0);
>>> +
>>> +        if (pid == s->qemu_pid && WIFSIGNALED(wstatus)) {
>>> +            assert(!WCOREDUMP(wstatus));
>>
>> Another ugliness that I just discovered: kill_qemu is also called from
>> the SIGABRT handler. So if a qtest assert() triggers an abort(), the
>> abort handler runs kill_qemu which now could trigger another assert()
>> and thus abort().
> 
> But only the first one will cause a coredump.
> 
>> It's likely not a real problem since the abort handler
>> has been installed with SA_RESETHAND, but it's still quite confusing code.
>>
>> Please let's clean up this ugliness properly: I think kill_qemu should
>> *only* be used by the abort handler, and then kill QEMU with SIGKILL for
>> good, to make sure that there are no stuck QEMU processes hanging around
>> anymore.
>>
>> qtest_quit() should simply try to quit QEMU via QMP instead, and then
>> check for WIFEXITED(wstatus) && !WEXITSTATUS(wstatus) instead of using
>> the kill_qemu() function.
>>
>>  Thomas
> 
> I think I'll drop the second patch for now. failing test on coredump
> is clearly correct. The rest can wait until someone has the energy
> to look into all the intricacies of signal handling.

Ok, sounds like a plan.

Acked-by: Thomas Huth <address@hidden>