[Qemu-devel] [PATCH 0/2] qtest: fix SIGABRT handler for multiple qtest instances

[Qemu-devel] [PATCH 0/2] qtest: fix SIGABRT handler for multiple qtest instances

[Stefan Hajnoczi]

I didn't realize some test cases use multiple qtest instances.  This means we
need to know about all instances in order to kill our QEMU processes.

Stefan Hajnoczi (2):
  Revert "qtest: Fix crash if SIGABRT during qtest_init()"
  qtest: keep list of qtest instances for SIGABRT handler

 tests/libqtest.c | 50 ++++++++++++++++++++++++++++++++++++++------------
 tests/libqtest.h |  4 +++-
 2 files changed, 41 insertions(+), 13 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH 1/2] Revert "qtest: Fix crash if SIGABRT during qtest_init()"

[Stefan Hajnoczi]

It turns out there are test cases that use multiple libqtest instances.
We cannot use a global qtest instance in the SIGABRT handler.

This reverts commit cb201b4872f16dfbce63f8648b2584631e2e965f.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqtest.c | 3 +--
 tests/libqtest.h | 4 +++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index b03b57a..2b90e4a 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -120,7 +120,7 @@ QTestState *qtest_init(const char *extra_args)
     qemu_binary = getenv("QTEST_QEMU_BINARY");
     g_assert(qemu_binary != NULL);
 
-    global_qtest = s = g_malloc(sizeof(*s));
+    s = g_malloc(sizeof(*s));
 
     socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
     qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
@@ -181,7 +181,6 @@ QTestState *qtest_init(const char *extra_args)
 void qtest_quit(QTestState *s)
 {
     sigaction(SIGABRT, &s->sigact_old, NULL);
-    global_qtest = NULL;
 
     kill_qemu(s);
     close(s->fd);
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 27a58fd..8268c09 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -335,7 +335,8 @@ void qtest_add_func(const char *str, void (*fn));
  */
 static inline QTestState *qtest_start(const char *args)
 {
-    return qtest_init(args);
+    global_qtest = qtest_init(args);
+    return global_qtest;
 }
 
 /**
@@ -346,6 +347,7 @@ static inline QTestState *qtest_start(const char *args)
 static inline void qtest_end(void)
 {
     qtest_quit(global_qtest);
+    global_qtest = NULL;
 }
 
 /**
-- 
1.8.5.3

[Qemu-devel] [PATCH 2/2] qtest: keep list of qtest instances for SIGABRT handler

[Stefan Hajnoczi]

Keep track of active qtest instances so we can kill them when the test
aborts.  This ensures no QEMU processes are left running after test
failure.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqtest.c | 47 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 2b90e4a..d9e3a33 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -48,6 +48,9 @@ struct QTestState
     struct sigaction sigact_old; /* restored on exit */
 };
 
+static GList *qtest_instances;
+static struct sigaction sigact_old;
+
 #define g_assert_no_errno(ret) do { \
     g_assert_cmpint(ret, !=, -1); \
 } while (0)
@@ -104,7 +107,28 @@ static void kill_qemu(QTestState *s)
 
 static void sigabrt_handler(int signo)
 {
-    kill_qemu(global_qtest);
+    GList *elem;
+    for (elem = qtest_instances; elem; elem = elem->next) {
+        kill_qemu(elem->data);
+    }
+}
+
+static void setup_sigabrt_handler(void)
+{
+    struct sigaction sigact;
+
+    /* Catch SIGABRT to clean up on g_assert() failure */
+    sigact = (struct sigaction){
+        .sa_handler = sigabrt_handler,
+        .sa_flags = SA_RESETHAND,
+    };
+    sigemptyset(&sigact.sa_mask);
+    sigaction(SIGABRT, &sigact, &sigact_old);
+}
+
+static void cleanup_sigabrt_handler(void)
+{
+    sigaction(SIGABRT, &sigact_old, NULL);
 }
 
 QTestState *qtest_init(const char *extra_args)
@@ -115,7 +139,6 @@ QTestState *qtest_init(const char *extra_args)
     gchar *qmp_socket_path;
     gchar *command;
     const char *qemu_binary;
-    struct sigaction sigact;
 
     qemu_binary = getenv("QTEST_QEMU_BINARY");
     g_assert(qemu_binary != NULL);
@@ -128,13 +151,12 @@ QTestState *qtest_init(const char *extra_args)
     sock = init_socket(socket_path);
     qmpsock = init_socket(qmp_socket_path);
 
-    /* Catch SIGABRT to clean up on g_assert() failure */
-    sigact = (struct sigaction){
-        .sa_handler = sigabrt_handler,
-        .sa_flags = SA_RESETHAND,
-    };
-    sigemptyset(&sigact.sa_mask);
-    sigaction(SIGABRT, &sigact, &s->sigact_old);
+    /* Only install SIGABRT handler once */
+    if (!qtest_instances) {
+        setup_sigabrt_handler();
+    }
+
+    qtest_instances = g_list_prepend(qtest_instances, s);
 
     s->qemu_pid = fork();
     if (s->qemu_pid == 0) {
@@ -180,7 +202,12 @@ QTestState *qtest_init(const char *extra_args)
 
 void qtest_quit(QTestState *s)
 {
-    sigaction(SIGABRT, &s->sigact_old, NULL);
+    /* Uninstall SIGABRT handler on last instance */
+    if (qtest_instances && !qtest_instances->next) {
+        cleanup_sigabrt_handler();
+    }
+
+    qtest_instances = g_list_remove(qtest_instances, s);
 
     kill_qemu(s);
     close(s->fd);
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH 0/2] qtest: fix SIGABRT handler for multiple qtest instances

[Marcel Apfelbaum]

On Thu, 2014-03-27 at 15:09 +0100, Stefan Hajnoczi wrote:
> I didn't realize some test cases use multiple qtest instances.  This means we
> need to know about all instances in order to kill our QEMU processes.
> 
> Stefan Hajnoczi (2):
>   Revert "qtest: Fix crash if SIGABRT during qtest_init()"
>   qtest: keep list of qtest instances for SIGABRT handler
> 
>  tests/libqtest.c | 50 ++++++++++++++++++++++++++++++++++++++------------
>  tests/libqtest.h |  4 +++-
>  2 files changed, 41 insertions(+), 13 deletions(-)
> 

Looks OK to me,

Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH 0/2] qtest: fix SIGABRT handler for multiple qtest instances

[Andreas Färber]

Am 27.03.2014 15:30, schrieb Marcel Apfelbaum:
> On Thu, 2014-03-27 at 15:09 +0100, Stefan Hajnoczi wrote:
>> I didn't realize some test cases use multiple qtest instances.  This means we
>> need to know about all instances in order to kill our QEMU processes.
>>
>> Stefan Hajnoczi (2):
>>   Revert "qtest: Fix crash if SIGABRT during qtest_init()"
>>   qtest: keep list of qtest instances for SIGABRT handler
>>
>>  tests/libqtest.c | 50 ++++++++++++++++++++++++++++++++++++++------------
>>  tests/libqtest.h |  4 +++-
>>  2 files changed, 41 insertions(+), 13 deletions(-)
>>
> 
> Looks OK to me,
> 
> Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>

Thanks, applied to qom-next:
https://github.com/afaerber/qemu-cpu/commits/qom-next

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg