1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v4 0/3] libqtest: verify QEMU exit status
  4. View patch

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

Michael S. Tsirkin posted 3 patches 7 years, 8 months ago
[Qemu-devel] [PATCH v4 2/3] libqtest: fail if child coredumps
Posted by Michael S. Tsirkin 7 years, 8 months ago 
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 <mst@redhat.com>
---
 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));
+        }
     }
 }
 
-- 
MST
Re: [Qemu-devel] [PATCH v4 2/3] libqtest: fail if child coredumps
Posted by Thomas Huth 7 years, 8 months ago 
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 <mst@redhat.com>
> ---
>  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(). 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
Re: [Qemu-devel] [PATCH v4 2/3] libqtest: fail if child coredumps
Posted by Thomas Huth 7 years, 8 months ago 
On 25.05.2018 08:10, 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 <mst@redhat.com>
>> ---
>>  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(). 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.

I just did some experiments with that, and using QMP 'quit' to exit QEMU
is also not working very reliable - some tests apparently mess up QMP
quite badly, so the 'quit' does not work during qtest_quit anymore.
Looks like we have to continue to send SIGTERM during qtest_quit(). But
I still think we should separate the logic from the abort handler (which
should use SIGKILL in case SIGTERM does not work as expected).

 Thomas
Re: [Qemu-devel] [PATCH v4 2/3] libqtest: fail if child coredumps
Posted by Michael S. Tsirkin 7 years, 8 months ago 
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 <mst@redhat.com>
> > ---
> >  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.

-- 
MST
Re: [Qemu-devel] [PATCH v4 2/3] libqtest: fail if child coredumps
Posted by Thomas Huth 7 years, 8 months ago 
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 <mst@redhat.com>
>>> ---
>>>  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 <thuth@redhat.com>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.