[PATCH v2 0/2] tests/libqtest: fix cleanup of QEMU processes and add robustness

[PATCH v2 0/2] tests/libqtest: fix cleanup of QEMU processes and add robustness

[Daniel P. Berrangé]

This fixes the existing logic that was supposed to be cleaning up
running QEMU processes when g_assert fires. It then further adds
use of a Linux specific prctl as a safety net if the ABRT handler
couldn't convince QEMU to quit with SIGTERM, or if the test program
crashes with SEGV.

Daniel P. Berrangé (2):
  tests/qtest: fix registration of ABRT handler for QEMU cleanup
  tests/qtest: use prctl(PR_SET_PDEATHSIG) as fallback to kill QEMU

 tests/qtest/libqtest.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

-- 
2.36.1

[PATCH v2 1/2] tests/qtest: fix registration of ABRT handler for QEMU cleanup

[Daniel P. Berrangé]

qtest_init registers a hook to cleanup the running QEMU process
should g_assert() fire before qtest_quit is called. When the first
hook is registered, it is supposed to triggere registration of the
SIGABRT handler. Unfortunately the logic in hook_list_is_empty is
inverted, so the SIGABRT handler never gets registered, unless
2 or more QEMU processes are run concurrently. This caused qtest
to leak QEMU processes anytime g_assert triggers.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qtest/libqtest.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 228357f1ea..4a4697c0d1 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -197,11 +197,11 @@ static bool hook_list_is_empty(GHookList *hook_list)
     GHook *hook = g_hook_first_valid(hook_list, TRUE);
 
     if (!hook) {
-        return false;
+        return true;
     }
 
     g_hook_unref(hook_list, hook);
-    return true;
+    return false;
 }
 
 void qtest_add_abrt_handler(GHookFunc fn, const void *data)
-- 
2.36.1

[PATCH v2 2/2] tests/qtest: use prctl(PR_SET_PDEATHSIG) as fallback to kill QEMU

[Daniel P. Berrangé]

Although we register a ABRT handler to kill off QEMU when g_assert()
triggers, we want an extra safety net. The QEMU process might be
non-functional and thus not have responded to SIGTERM. The test script
might also have crashed with SEGV, in which case the cleanup handlers
won't ever run.

Using the Linux specific prctl(PR_SET_PDEATHSIG) syscall, we
can ensure that QEMU gets sent SIGKILL as soon as the controlling
qtest exits, if nothing else has correctly told it to quit.

Note, technically the death signal is sent when the *thread* that
called fork() exits. IOW, if you are calling qtest_init() in one
thread, letting that thread exit, and then expecting to run
qtest_quit() in a different thread, things are not going to work
out. Fortunately that is not a scenario that exists in qtests,
as pairs of qtest_init and qtest_quit are always called from the
same thread.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qtest/libqtest.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 4a4697c0d1..2e49618454 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -19,6 +19,9 @@
 #include <sys/socket.h>
 #include <sys/wait.h>
 #include <sys/un.h>
+#ifdef __linux__
+#include <sys/prctl.h>
+#endif /* __linux__ */
 
 #include "libqtest.h"
 #include "libqmp.h"
@@ -301,6 +304,20 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
     s->expected_status = 0;
     s->qemu_pid = fork();
     if (s->qemu_pid == 0) {
+#ifdef __linux__
+        /*
+         * Although we register a ABRT handler to kill off QEMU
+         * when g_assert() triggers, we want an extra safety
+         * net. The QEMU process might be non-functional and
+         * thus not have responded to SIGTERM. The test script
+         * might also have crashed with SEGV, in which case the
+         * cleanup handlers won't ever run.
+         *
+         * This PR_SET_PDEATHSIG setup will ensure any remaining
+         * QEMU will get terminated with SIGKILL in these cases.
+         */
+        prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0);
+#endif /* __linux__ */
         if (!g_setenv("QEMU_AUDIO_DRV", "none", true)) {
             exit(1);
         }
-- 
2.36.1

Re: [PATCH v2 1/2] tests/qtest: fix registration of ABRT handler for QEMU cleanup

[Thomas Huth]

On 13/05/2022 17.49, Daniel P. Berrangé wrote:
> qtest_init registers a hook to cleanup the running QEMU process
> should g_assert() fire before qtest_quit is called. When the first
> hook is registered, it is supposed to triggere registration of the
> SIGABRT handler. Unfortunately the logic in hook_list_is_empty is
> inverted, so the SIGABRT handler never gets registered, unless
> 2 or more QEMU processes are run concurrently. This caused qtest
> to leak QEMU processes anytime g_assert triggers.

Ouch, thanks for spotting it!

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qtest/libqtest.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 228357f1ea..4a4697c0d1 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -197,11 +197,11 @@ static bool hook_list_is_empty(GHookList *hook_list)
>       GHook *hook = g_hook_first_valid(hook_list, TRUE);
>   
>       if (!hook) {
> -        return false;
> +        return true;
>       }
>   
>       g_hook_unref(hook_list, hook);
> -    return true;
> +    return false;
>   }

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 2/2] tests/qtest: use prctl(PR_SET_PDEATHSIG) as fallback to kill QEMU

[Thomas Huth]

On 13/05/2022 17.49, Daniel P. Berrangé wrote:
> Although we register a ABRT handler to kill off QEMU when g_assert()
> triggers, we want an extra safety net. The QEMU process might be
> non-functional and thus not have responded to SIGTERM. The test script
> might also have crashed with SEGV, in which case the cleanup handlers
> won't ever run.
> 
> Using the Linux specific prctl(PR_SET_PDEATHSIG) syscall, we
> can ensure that QEMU gets sent SIGKILL as soon as the controlling
> qtest exits, if nothing else has correctly told it to quit.
> 
> Note, technically the death signal is sent when the *thread* that
> called fork() exits. IOW, if you are calling qtest_init() in one
> thread, letting that thread exit, and then expecting to run
> qtest_quit() in a different thread, things are not going to work
> out. Fortunately that is not a scenario that exists in qtests,
> as pairs of qtest_init and qtest_quit are always called from the
> same thread.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qtest/libqtest.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 4a4697c0d1..2e49618454 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -19,6 +19,9 @@
>   #include <sys/socket.h>
>   #include <sys/wait.h>
>   #include <sys/un.h>
> +#ifdef __linux__
> +#include <sys/prctl.h>
> +#endif /* __linux__ */
>   
>   #include "libqtest.h"
>   #include "libqmp.h"
> @@ -301,6 +304,20 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
>       s->expected_status = 0;
>       s->qemu_pid = fork();
>       if (s->qemu_pid == 0) {
> +#ifdef __linux__
> +        /*
> +         * Although we register a ABRT handler to kill off QEMU
> +         * when g_assert() triggers, we want an extra safety
> +         * net. The QEMU process might be non-functional and
> +         * thus not have responded to SIGTERM. The test script
> +         * might also have crashed with SEGV, in which case the
> +         * cleanup handlers won't ever run.
> +         *
> +         * This PR_SET_PDEATHSIG setup will ensure any remaining
> +         * QEMU will get terminated with SIGKILL in these cases.
> +         */
> +        prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0);
> +#endif /* __linux__ */
>           if (!g_setenv("QEMU_AUDIO_DRV", "none", true)) {
>               exit(1);
>           }

Reviewed-by: Thomas Huth <thuth@redhat.com>