[Qemu-devel] [PATCH v4 0/3] libqtest: verify QEMU exit status

[Qemu-devel] [PATCH v4 0/3] libqtest: verify QEMU exit status

[Michael S. Tsirkin]

Whenever QEMU coredumps, the test would previously succeed.
With this patchset applied, one sees:
    assertion failed: !WCOREDUMP(wstatus)

Changes from v3:
- add osdep stubs for non-linux platforms, suggested by Thomas

Changes from v2:
- bugfix
- assert returned pid
- rework complex asserts for clarity

Changes from v1:
- drop SIGTERM as suggested by Eric

Michael S. Tsirkin (3):
  osdep: add wait.h compat macros
  libqtest: fail if child coredumps
  libqtest: add more exit status checks

 include/qemu/osdep.h | 10 ++++++++++
 tests/libqtest.c     | 19 ++++++++++++++++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

-- 
MST

[Qemu-devel] [PATCH v4 1/3] osdep: add wait.h compat macros

[Michael S. Tsirkin]

Man page for WCOREDUMP says:

  WCOREDUMP(wstatus) returns true if the child produced a core dump.
  This macro should be employed only if WIFSIGNALED returned true.

  This  macro  is  not  specified  in POSIX.1-2001 and is not
  available on some UNIX implementations (e.g., AIX, SunOS).  Therefore,
  enclose its use inside #ifdef WCOREDUMP ... #endif.

Let's do exactly this.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/qemu/osdep.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 4165806..afc28e5 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -108,6 +108,16 @@ extern int daemon(int, int);
 #include "qemu/typedefs.h"
 
 /*
+ * According to waitpid man page:
+ * WCOREDUMP
+ *  This  macro  is  not  specified  in POSIX.1-2001 and is not
+ *  available on some UNIX implementations (e.g., AIX, SunOS).
+ *  Therefore, enclose its use inside #ifdef WCOREDUMP ... #endif.
+ */
+#ifndef WCOREDUMP
+#define WCOREDUMP(status) 0
+#endif
+/*
  * We have a lot of unaudited code that may fail in strange ways, or
  * even be a security risk during migration, if you disable assertions
  * at compile-time.  You may comment out these safety checks if you
-- 
MST

Re: [Qemu-devel] [PATCH v4 1/3] osdep: add wait.h compat macros

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> Man page for WCOREDUMP says:
>
>   WCOREDUMP(wstatus) returns true if the child produced a core dump.
>   This macro should be employed only if WIFSIGNALED returned true.
>
>   This  macro  is  not  specified  in POSIX.1-2001 and is not
>   available on some UNIX implementations (e.g., AIX, SunOS).  Therefore,
>   enclose its use inside #ifdef WCOREDUMP ... #endif.
>
> Let's do exactly this.

Sorry to nitpick, but you're not doing "exactly this", you're providing
a stub:

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/qemu/osdep.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 4165806..afc28e5 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -108,6 +108,16 @@ extern int daemon(int, int);
>  #include "qemu/typedefs.h"
>  
>  /*
> + * According to waitpid man page:
> + * WCOREDUMP
> + *  This  macro  is  not  specified  in POSIX.1-2001 and is not
> + *  available on some UNIX implementations (e.g., AIX, SunOS).
> + *  Therefore, enclose its use inside #ifdef WCOREDUMP ... #endif.
> + */
> +#ifndef WCOREDUMP
> +#define WCOREDUMP(status) 0
> +#endif
> +/*
>   * We have a lot of unaudited code that may fail in strange ways, or
>   * even be a security risk during migration, if you disable assertions
>   * at compile-time.  You may comment out these safety checks if you

Suggest to drop the last sentence from the comment, and have the commit
message state you're providing a stub.

Re: [Qemu-devel] [PATCH v4 1/3] osdep: add wait.h compat macros

[Peter Maydell]

On 14 June 2018 at 13:56, Markus Armbruster <armbru@redhat.com> wrote:
> Suggest to drop the last sentence from the comment, and have the commit
> message state you're providing a stub.

This patch is already in upstream, commit 28012e190e68...

thanks
-- PMM

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

[Michael S. Tsirkin]

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

[Thomas Huth]

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

[Thomas Huth]

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

[Michael S. Tsirkin]

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

[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 <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>

[Qemu-devel] [PATCH v4 3/3] libqtest: add more exit status checks

[Michael S. Tsirkin]

Add more checks on how did QEMU exit.

Legal ways to exit right now:
- exit(0) or return from main
- kill(SIGTERM) - sent by testing infrastructure

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/libqtest.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index f869854..36ca859 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -109,9 +109,19 @@ static void kill_qemu(QTestState *s)
         kill(s->qemu_pid, SIGTERM);
         pid = waitpid(s->qemu_pid, &wstatus, 0);
 
-        if (pid == s->qemu_pid && WIFSIGNALED(wstatus)) {
+        /* waitpid returns child PID on success */
+        assert(pid == s->qemu_pid);
+
+        /* If exited on signal - check the reason: core dump is never OK */
+        if (WIFSIGNALED(wstatus)) {
             assert(!WCOREDUMP(wstatus));
         }
+        /* If exited normally - check exit status */
+        if (WIFEXITED(wstatus)) {
+            assert(!WEXITSTATUS(wstatus));
+        }
+        /* Valid ways to exit: right now only return from main or exit */
+        assert(WIFEXITED(wstatus));
     }
 }
 
-- 
MST

Re: [Qemu-devel] [PATCH v4 3/3] libqtest: add more exit status checks

[Michael S. Tsirkin]

On Thu, May 24, 2018 at 09:25:28PM +0300, Michael S. Tsirkin wrote:
> Add more checks on how did QEMU exit.
> 
> Legal ways to exit right now:
> - exit(0) or return from main
> - kill(SIGTERM) - sent by testing infrastructure
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---

This turned out to be messy since
- abort itself might trigger a signal
- waitpid might in theory get interrupted

I'll drop this patch for now, and merge just patches 1 and 2
in my tree.

>  tests/libqtest.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index f869854..36ca859 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -109,9 +109,19 @@ static void kill_qemu(QTestState *s)
>          kill(s->qemu_pid, SIGTERM);
>          pid = waitpid(s->qemu_pid, &wstatus, 0);
>  
> -        if (pid == s->qemu_pid && WIFSIGNALED(wstatus)) {
> +        /* waitpid returns child PID on success */
> +        assert(pid == s->qemu_pid);
> +
> +        /* If exited on signal - check the reason: core dump is never OK */
> +        if (WIFSIGNALED(wstatus)) {
>              assert(!WCOREDUMP(wstatus));
>          }
> +        /* If exited normally - check exit status */
> +        if (WIFEXITED(wstatus)) {
> +            assert(!WEXITSTATUS(wstatus));
> +        }
> +        /* Valid ways to exit: right now only return from main or exit */
> +        assert(WIFEXITED(wstatus));
>      }
>  }
>  
> -- 
> MST
>