[Qemu-devel] [PATCH 0/2] qtest: crash fix and improved "make check" output

[Qemu-devel] [PATCH 0/2] qtest: crash fix and improved "make check" output

[Stefan Hajnoczi]

These patches stem from the Marcel's discussion about qtest_init() failures.
He posted the qtest_init() deadlock fix and I was picky about how to fix two
other issues, so here are the patches.

Stefan Hajnoczi (2):
  tests: show the name of each executing qtest
  qtest: fix crash if SIGABRT during qtest_init()

 tests/Makefile   | 7 +++++--
 tests/libqtest.c | 3 ++-
 tests/libqtest.h | 4 +---
 3 files changed, 8 insertions(+), 6 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH 1/2] tests: show the name of each executing qtest

[Stefan Hajnoczi]

When a qtest fails only the assertion failure is printed but you do not
know which qtest binary was running:

  GTESTER check-qtest-x86_64
  main-loop: WARNING: I/O thread spun for 1000 iterations
  blkdebug: Suspended request 'A'
  blkdebug: Resuming request 'A'

check-qtest-x86_64 is actually a make target and not a gtester binary.
The make target includes over 20 separate qtest binaries.

The name of each executing qtest binary should be displayed:

  GTESTER tests/fdc-test
  main-loop: WARNING: I/O thread spun for 1000 iterations
  GTESTER tests/ide-test
  blkdebug: Suspended request 'A'
  blkdebug: Resuming request 'A'

This makes it easy to identify the failing test.

I tried out different ways of displaying qtest binary names.  This patch
implements the best (working) approach I found.  It generates a long
shell command joined with && to execute each qtest binary and print its
name.

This solution is ugly because it doesn't reuse quiet-command.  Maybe a
GNU Make guru will be able to use $(eval) to solve this, but I ended up
with a mix of shell and $(foreach).

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/Makefile | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index b17d41e..d80112a 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -281,9 +281,12 @@ GCOV_OPTIONS = -n $(if $(V),-f,)
 .PHONY: $(patsubst %, check-qtest-%, $(QTEST_TARGETS))
 $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)
 	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
-	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
+	@true $(foreach qtest-binary, $(check-qtest-$*-y), \
+		&& echo "GTESTER $(qtest-binary)" && \
+		QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
 		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \
-		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y),"GTESTER $@")
+		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(qtest-binary) \
+	)
 	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \
 	  echo Gcov report for $$f:;\
 	  $(GCOV) $(GCOV_OPTIONS) $$f -o `dirname $$f`; \
-- 
1.8.5.3

[Qemu-devel] [PATCH 2/2] qtest: fix crash if SIGABRT during qtest_init()

[Stefan Hajnoczi]

If an assertion fails during qtest_init() the SIGABRT handler is
invoked.  This is the correct behavior since we need to kill the QEMU
process to avoid leaking it when the test dies.

The global_qtest pointer used by the SIGABRT handler is currently only
assigned after qtest_init() returns.  This results in a segfault if an
assertion failure occurs during qtest_init().

Move global_qtest assignment inside qtest_init().  Not pretty but let's
face it - the signal handler dependeds on global state.

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

diff --git a/tests/libqtest.c b/tests/libqtest.c
index c9e78aa..f387662 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);
 
-    s = g_malloc(sizeof(*s));
+    global_qtest = 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,6 +181,7 @@ 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 9deebdc..7e23a4e 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -335,8 +335,7 @@ void qtest_add_func(const char *str, void (*fn));
  */
 static inline QTestState *qtest_start(const char *args)
 {
-    global_qtest = qtest_init(args);
-    return global_qtest;
+    return qtest_init(args);
 }
 
 /**
@@ -347,7 +346,6 @@ 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

Re: [Qemu-devel] [PATCH 2/2] qtest: fix crash if SIGABRT during qtest_init()

[Marcel Apfelbaum]

On Thu, 2014-03-13 at 10:41 +0100, Stefan Hajnoczi wrote:
> If an assertion fails during qtest_init() the SIGABRT handler is
> invoked.  This is the correct behavior since we need to kill the QEMU
> process to avoid leaking it when the test dies.
> 
> The global_qtest pointer used by the SIGABRT handler is currently only
> assigned after qtest_init() returns.  This results in a segfault if an
> assertion failure occurs during qtest_init().
> 
> Move global_qtest assignment inside qtest_init().  Not pretty but let's
> face it - the signal handler dependeds on global state.
Looks OK to me, but it seems that it is symmetrical with my
patch: Mine checked for global_qtest that is not null (not hiding anything :()
and yours increases global_qtest's scope.

I understand why you preferred it this way, to ensure the QEMU instance
is killed, but as I stated before, from my point of view
qtest_init aborted <=> the qemu machine exited because of on error.
(but I might be wrong)

Thanks,
Marcel

> 
> Reported-by: Marcel Apfelbaum <marcel.a@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqtest.c | 3 ++-
>  tests/libqtest.h | 4 +---
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index c9e78aa..f387662 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);
>  
> -    s = g_malloc(sizeof(*s));
> +    global_qtest = 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,6 +181,7 @@ 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 9deebdc..7e23a4e 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -335,8 +335,7 @@ void qtest_add_func(const char *str, void (*fn));
>   */
>  static inline QTestState *qtest_start(const char *args)
>  {
> -    global_qtest = qtest_init(args);
> -    return global_qtest;
> +    return qtest_init(args);
>  }
>  
>  /**
> @@ -347,7 +346,6 @@ static inline QTestState *qtest_start(const char *args)
>  static inline void qtest_end(void)
>  {
>      qtest_quit(global_qtest);
> -    global_qtest = NULL;
>  }
>  
>  /**

Re: [Qemu-devel] [PATCH 0/2] qtest: crash fix and improved "make check" output

[Marcel Apfelbaum]

On Thu, 2014-03-13 at 10:41 +0100, Stefan Hajnoczi wrote:
> These patches stem from the Marcel's discussion about qtest_init() failures.
> He posted the qtest_init() deadlock fix and I was picky about how to fix two
> other issues, so here are the patches.

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

> 
> Stefan Hajnoczi (2):
>   tests: show the name of each executing qtest
>   qtest: fix crash if SIGABRT during qtest_init()
> 
>  tests/Makefile   | 7 +++++--
>  tests/libqtest.c | 3 ++-
>  tests/libqtest.h | 4 +---
>  3 files changed, 8 insertions(+), 6 deletions(-)
> 

Re: [Qemu-devel] [PATCH 2/2] qtest: fix crash if SIGABRT during qtest_init()

[Stefan Hajnoczi]

On Thu, Mar 13, 2014 at 01:07:05PM +0200, Marcel Apfelbaum wrote:
> On Thu, 2014-03-13 at 10:41 +0100, Stefan Hajnoczi wrote:
> > If an assertion fails during qtest_init() the SIGABRT handler is
> > invoked.  This is the correct behavior since we need to kill the QEMU
> > process to avoid leaking it when the test dies.
> > 
> > The global_qtest pointer used by the SIGABRT handler is currently only
> > assigned after qtest_init() returns.  This results in a segfault if an
> > assertion failure occurs during qtest_init().
> > 
> > Move global_qtest assignment inside qtest_init().  Not pretty but let's
> > face it - the signal handler dependeds on global state.
> Looks OK to me, but it seems that it is symmetrical with my
> patch: Mine checked for global_qtest that is not null (not hiding anything :()
> and yours increases global_qtest's scope.
> 
> I understand why you preferred it this way, to ensure the QEMU instance
> is killed, but as I stated before, from my point of view
> qtest_init aborted <=> the qemu machine exited because of on error.
> (but I might be wrong)

Think about this case:

If we hit an assertion failure in qtest_init() because of socket errors
(e.g. QEMU ran for a little bit but closed the socket while we were
negotiating), then we *do* need to kill the QEMU process.

Stefan

Re: [Qemu-devel] [PATCH 2/2] qtest: fix crash if SIGABRT during qtest_init()

[Andreas Färber]

Am 13.03.2014 10:41, schrieb Stefan Hajnoczi:
> If an assertion fails during qtest_init() the SIGABRT handler is
> invoked.  This is the correct behavior since we need to kill the QEMU
> process to avoid leaking it when the test dies.
> 
> The global_qtest pointer used by the SIGABRT handler is currently only
> assigned after qtest_init() returns.  This results in a segfault if an
> assertion failure occurs during qtest_init().
> 
> Move global_qtest assignment inside qtest_init().  Not pretty but let's
> face it - the signal handler dependeds on global state.
> 
> Reported-by: Marcel Apfelbaum <marcel.a@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqtest.c | 3 ++-
>  tests/libqtest.h | 4 +---
>  2 files changed, 3 insertions(+), 4 deletions(-)

Thanks, applied to qom-next (with typo fix):
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

Re: [Qemu-devel] [PATCH 2/2] qtest: fix crash if SIGABRT during qtest_init()

[Markus Armbruster]

Reply after commit, sorry.

Stefan Hajnoczi <stefanha@redhat.com> writes:

> If an assertion fails during qtest_init() the SIGABRT handler is
> invoked.  This is the correct behavior since we need to kill the QEMU
> process to avoid leaking it when the test dies.
>
> The global_qtest pointer used by the SIGABRT handler is currently only
> assigned after qtest_init() returns.  This results in a segfault if an
> assertion failure occurs during qtest_init().
>
> Move global_qtest assignment inside qtest_init().  Not pretty but let's
> face it - the signal handler dependeds on global state.
>
> Reported-by: Marcel Apfelbaum <marcel.a@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqtest.c | 3 ++-
>  tests/libqtest.h | 4 +---
>  2 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index c9e78aa..f387662 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);
>  
> -    s = g_malloc(sizeof(*s));
> +    global_qtest = 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,6 +181,7 @@ 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 9deebdc..7e23a4e 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -335,8 +335,7 @@ void qtest_add_func(const char *str, void (*fn));
>   */
>  static inline QTestState *qtest_start(const char *args)
>  {
> -    global_qtest = qtest_init(args);
> -    return global_qtest;
> +    return qtest_init(args);
>  }
>  
>  /**
> @@ -347,7 +346,6 @@ static inline QTestState *qtest_start(const char *args)
>  static inline void qtest_end(void)
>  {
>      qtest_quit(global_qtest);
> -    global_qtest = NULL;
>  }
>  
>  /**

Before this patch, the libqtest API could theoretically support multiple
simultaneous instances of QTestState.  This patch kills that option,
doesn't it?

If yes: fine with me, we don't need it anyway.  But shouldn't we clean
up the API then?  It typically provides a function taking a QTestState
parameter, and a wrapper that passes global_qtest.  If global_qtest is
the only QTestState we can have, having the former function is
pointless.

Re: [Qemu-devel] [PATCH 2/2] qtest: fix crash if SIGABRT during qtest_init()

[Andreas Färber]

Am 27.03.2014 14:09, schrieb Markus Armbruster:
> Reply after commit, sorry.
> 
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
>> If an assertion fails during qtest_init() the SIGABRT handler is
>> invoked.  This is the correct behavior since we need to kill the QEMU
>> process to avoid leaking it when the test dies.
>>
>> The global_qtest pointer used by the SIGABRT handler is currently only
>> assigned after qtest_init() returns.  This results in a segfault if an
>> assertion failure occurs during qtest_init().
>>
>> Move global_qtest assignment inside qtest_init().  Not pretty but let's
>> face it - the signal handler dependeds on global state.
>>
>> Reported-by: Marcel Apfelbaum <marcel.a@redhat.com>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  tests/libqtest.c | 3 ++-
>>  tests/libqtest.h | 4 +---
>>  2 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index c9e78aa..f387662 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);
>>  
>> -    s = g_malloc(sizeof(*s));
>> +    global_qtest = 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,6 +181,7 @@ 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 9deebdc..7e23a4e 100644
>> --- a/tests/libqtest.h
>> +++ b/tests/libqtest.h
>> @@ -335,8 +335,7 @@ void qtest_add_func(const char *str, void (*fn));
>>   */
>>  static inline QTestState *qtest_start(const char *args)
>>  {
>> -    global_qtest = qtest_init(args);
>> -    return global_qtest;
>> +    return qtest_init(args);
>>  }
>>  
>>  /**
>> @@ -347,7 +346,6 @@ static inline QTestState *qtest_start(const char *args)
>>  static inline void qtest_end(void)
>>  {
>>      qtest_quit(global_qtest);
>> -    global_qtest = NULL;
>>  }
>>  
>>  /**
> 
> Before this patch, the libqtest API could theoretically support multiple
> simultaneous instances of QTestState.  This patch kills that option,
> doesn't it?

Ouch, I thought I had looked out for that...

> 
> If yes: fine with me, we don't need it anyway.

We do. Migration and ivshmem are examples that need two machines - might
explain why my ivshmem-test was behaving unexpectedly.

Apart from reverting, what are our options?

Regards,
Andreas

>  But shouldn't we clean
> up the API then?  It typically provides a function taking a QTestState
> parameter, and a wrapper that passes global_qtest.  If global_qtest is
> the only QTestState we can have, having the former function is
> pointless.
> 


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

Re: [Qemu-devel] [PATCH 2/2] qtest: fix crash if SIGABRT during qtest_init()

[Marcel Apfelbaum]

On Thu, 2014-03-27 at 14:12 +0100, Andreas Färber wrote:
> Am 27.03.2014 14:09, schrieb Markus Armbruster:
> > Reply after commit, sorry.
> > 
> > Stefan Hajnoczi <stefanha@redhat.com> writes:
> > 
> >> If an assertion fails during qtest_init() the SIGABRT handler is
> >> invoked.  This is the correct behavior since we need to kill the QEMU
> >> process to avoid leaking it when the test dies.
> >>
> >> The global_qtest pointer used by the SIGABRT handler is currently only
> >> assigned after qtest_init() returns.  This results in a segfault if an
> >> assertion failure occurs during qtest_init().
> >>
> >> Move global_qtest assignment inside qtest_init().  Not pretty but let's
> >> face it - the signal handler dependeds on global state.
> >>
> >> Reported-by: Marcel Apfelbaum <marcel.a@redhat.com>
> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> ---
> >>  tests/libqtest.c | 3 ++-
> >>  tests/libqtest.h | 4 +---
> >>  2 files changed, 3 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tests/libqtest.c b/tests/libqtest.c
> >> index c9e78aa..f387662 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);
> >>  
> >> -    s = g_malloc(sizeof(*s));
> >> +    global_qtest = 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,6 +181,7 @@ 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 9deebdc..7e23a4e 100644
> >> --- a/tests/libqtest.h
> >> +++ b/tests/libqtest.h
> >> @@ -335,8 +335,7 @@ void qtest_add_func(const char *str, void (*fn));
> >>   */
> >>  static inline QTestState *qtest_start(const char *args)
> >>  {
> >> -    global_qtest = qtest_init(args);
> >> -    return global_qtest;
> >> +    return qtest_init(args);
> >>  }
> >>  
> >>  /**
> >> @@ -347,7 +346,6 @@ static inline QTestState *qtest_start(const char *args)
> >>  static inline void qtest_end(void)
> >>  {
> >>      qtest_quit(global_qtest);
> >> -    global_qtest = NULL;
> >>  }
> >>  
> >>  /**
> > 
> > Before this patch, the libqtest API could theoretically support multiple
> > simultaneous instances of QTestState.  This patch kills that option,
> > doesn't it?
> 
> Ouch, I thought I had looked out for that...
> 
> > 
> > If yes: fine with me, we don't need it anyway.
> 
> We do. Migration and ivshmem are examples that need two machines - might
> explain why my ivshmem-test was behaving unexpectedly.
> 
> Apart from reverting, what are our options?
The problem is in 'kill_qemu' function, which is called from
SIGABRT signal handler.  The later needs to know the QTestState
in order to kill the QEMU process.

Without this patch, kill_qemu will cause a segfault because of:
static void kill_qemu(QTestState *s)
{
    if (s->qemu_pid != -1) {
...
s can be NULL if there is an assert in qtest_init.

I suppose we can find a different approach, like keeping
the qemu_pid(s) in another statically defined struct.
 
Thanks,
Marcel

 
> 
> Regards,
> Andreas
> 
> >  But shouldn't we clean
> > up the API then?  It typically provides a function taking a QTestState
> > parameter, and a wrapper that passes global_qtest.  If global_qtest is
> > the only QTestState we can have, having the former function is
> > pointless.
> > 
> 
> 

Re: [Qemu-devel] [PATCH 2/2] qtest: fix crash if SIGABRT during qtest_init()

[Stefan Hajnoczi]

On Thu, Mar 27, 2014 at 2:12 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Before this patch, the libqtest API could theoretically support multiple
>> simultaneous instances of QTestState.  This patch kills that option,
>> doesn't it?
>
> Ouch, I thought I had looked out for that...
>
>>
>> If yes: fine with me, we don't need it anyway.
>
> We do. Migration and ivshmem are examples that need two machines - might
> explain why my ivshmem-test was behaving unexpectedly.
>
> Apart from reverting, what are our options?

Argh, I wasn't aware some tests run with two separate instances.

We can implement more elaborate error handling, for example an
atexit(3)-style atabort mechanism.  This way, each instance can get
its callback.

Stefan

Re: [Qemu-devel] [PATCH 2/2] qtest: fix crash if SIGABRT during qtest_init()

[Stefan Hajnoczi]

On Thu, Mar 27, 2014 at 2:34 PM, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> On Thu, 2014-03-27 at 14:12 +0100, Andreas Färber wrote:
>> Am 27.03.2014 14:09, schrieb Markus Armbruster:
>> > Reply after commit, sorry.
>> >
>> > Stefan Hajnoczi <stefanha@redhat.com> writes:
>> >
>> >> If an assertion fails during qtest_init() the SIGABRT handler is
>> >> invoked.  This is the correct behavior since we need to kill the QEMU
>> >> process to avoid leaking it when the test dies.
>> >>
>> >> The global_qtest pointer used by the SIGABRT handler is currently only
>> >> assigned after qtest_init() returns.  This results in a segfault if an
>> >> assertion failure occurs during qtest_init().
>> >>
>> >> Move global_qtest assignment inside qtest_init().  Not pretty but let's
>> >> face it - the signal handler dependeds on global state.
>> >>
>> >> Reported-by: Marcel Apfelbaum <marcel.a@redhat.com>
>> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> >> ---
>> >>  tests/libqtest.c | 3 ++-
>> >>  tests/libqtest.h | 4 +---
>> >>  2 files changed, 3 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> >> index c9e78aa..f387662 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);
>> >>
>> >> -    s = g_malloc(sizeof(*s));
>> >> +    global_qtest = 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,6 +181,7 @@ 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 9deebdc..7e23a4e 100644
>> >> --- a/tests/libqtest.h
>> >> +++ b/tests/libqtest.h
>> >> @@ -335,8 +335,7 @@ void qtest_add_func(const char *str, void (*fn));
>> >>   */
>> >>  static inline QTestState *qtest_start(const char *args)
>> >>  {
>> >> -    global_qtest = qtest_init(args);
>> >> -    return global_qtest;
>> >> +    return qtest_init(args);
>> >>  }
>> >>
>> >>  /**
>> >> @@ -347,7 +346,6 @@ static inline QTestState *qtest_start(const char *args)
>> >>  static inline void qtest_end(void)
>> >>  {
>> >>      qtest_quit(global_qtest);
>> >> -    global_qtest = NULL;
>> >>  }
>> >>
>> >>  /**
>> >
>> > Before this patch, the libqtest API could theoretically support multiple
>> > simultaneous instances of QTestState.  This patch kills that option,
>> > doesn't it?
>>
>> Ouch, I thought I had looked out for that...
>>
>> >
>> > If yes: fine with me, we don't need it anyway.
>>
>> We do. Migration and ivshmem are examples that need two machines - might
>> explain why my ivshmem-test was behaving unexpectedly.
>>
>> Apart from reverting, what are our options?
> The problem is in 'kill_qemu' function, which is called from
> SIGABRT signal handler.  The later needs to know the QTestState
> in order to kill the QEMU process.
>
> Without this patch, kill_qemu will cause a segfault because of:
> static void kill_qemu(QTestState *s)
> {
>     if (s->qemu_pid != -1) {
> ...
> s can be NULL if there is an assert in qtest_init.
>
> I suppose we can find a different approach, like keeping
> the qemu_pid(s) in another statically defined struct.

We can keep a global linked list of QEMU pids.

Stefan

Re: [Qemu-devel] [PATCH 2/2] qtest: fix crash if SIGABRT during qtest_init()

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Thu, Mar 27, 2014 at 2:34 PM, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
>> On Thu, 2014-03-27 at 14:12 +0100, Andreas Färber wrote:
>>> Am 27.03.2014 14:09, schrieb Markus Armbruster:
>>> > Reply after commit, sorry.
>>> >
>>> > Stefan Hajnoczi <stefanha@redhat.com> writes:
>>> >
>>> >> If an assertion fails during qtest_init() the SIGABRT handler is
>>> >> invoked.  This is the correct behavior since we need to kill the QEMU
>>> >> process to avoid leaking it when the test dies.
>>> >>
>>> >> The global_qtest pointer used by the SIGABRT handler is currently only
>>> >> assigned after qtest_init() returns.  This results in a segfault if an
>>> >> assertion failure occurs during qtest_init().
>>> >>
>>> >> Move global_qtest assignment inside qtest_init().  Not pretty but let's
>>> >> face it - the signal handler dependeds on global state.
>>> >>
>>> >> Reported-by: Marcel Apfelbaum <marcel.a@redhat.com>
>>> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> >> ---
>>> >>  tests/libqtest.c | 3 ++-
>>> >>  tests/libqtest.h | 4 +---
>>> >>  2 files changed, 3 insertions(+), 4 deletions(-)
>>> >>
>>> >> diff --git a/tests/libqtest.c b/tests/libqtest.c
>>> >> index c9e78aa..f387662 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);
>>> >>
>>> >> -    s = g_malloc(sizeof(*s));
>>> >> +    global_qtest = 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,6 +181,7 @@ 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 9deebdc..7e23a4e 100644
>>> >> --- a/tests/libqtest.h
>>> >> +++ b/tests/libqtest.h
>>> >> @@ -335,8 +335,7 @@ void qtest_add_func(const char *str, void (*fn));
>>> >>   */
>>> >>  static inline QTestState *qtest_start(const char *args)
>>> >>  {
>>> >> -    global_qtest = qtest_init(args);
>>> >> -    return global_qtest;
>>> >> +    return qtest_init(args);
>>> >>  }
>>> >>
>>> >>  /**
>>> >> @@ -347,7 +346,6 @@ static inline QTestState *qtest_start(const char *args)
>>> >>  static inline void qtest_end(void)
>>> >>  {
>>> >>      qtest_quit(global_qtest);
>>> >> -    global_qtest = NULL;
>>> >>  }
>>> >>
>>> >>  /**
>>> >
>>> > Before this patch, the libqtest API could theoretically support multiple
>>> > simultaneous instances of QTestState.  This patch kills that option,
>>> > doesn't it?
>>>
>>> Ouch, I thought I had looked out for that...
>>>
>>> >
>>> > If yes: fine with me, we don't need it anyway.
>>>
>>> We do. Migration and ivshmem are examples that need two machines - might
>>> explain why my ivshmem-test was behaving unexpectedly.
>>>
>>> Apart from reverting, what are our options?
>> The problem is in 'kill_qemu' function, which is called from
>> SIGABRT signal handler.  The later needs to know the QTestState
>> in order to kill the QEMU process.
>>
>> Without this patch, kill_qemu will cause a segfault because of:
>> static void kill_qemu(QTestState *s)
>> {
>>     if (s->qemu_pid != -1) {
>> ...
>> s can be NULL if there is an assert in qtest_init.
>>
>> I suppose we can find a different approach, like keeping
>> the qemu_pid(s) in another statically defined struct.
>
> We can keep a global linked list of QEMU pids.

What about a global list of QTestState?

Re: [Qemu-devel] [PATCH 2/2] qtest: fix crash if SIGABRT during qtest_init()

[Stefan Hajnoczi]

On Thu, Mar 27, 2014 at 3:02 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
>
>> On Thu, Mar 27, 2014 at 2:34 PM, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
>>> On Thu, 2014-03-27 at 14:12 +0100, Andreas Färber wrote:
>>>> Am 27.03.2014 14:09, schrieb Markus Armbruster:
>>>> > Reply after commit, sorry.
>>>> >
>>>> > Stefan Hajnoczi <stefanha@redhat.com> writes:
>>>> >
>>>> >> If an assertion fails during qtest_init() the SIGABRT handler is
>>>> >> invoked.  This is the correct behavior since we need to kill the QEMU
>>>> >> process to avoid leaking it when the test dies.
>>>> >>
>>>> >> The global_qtest pointer used by the SIGABRT handler is currently only
>>>> >> assigned after qtest_init() returns.  This results in a segfault if an
>>>> >> assertion failure occurs during qtest_init().
>>>> >>
>>>> >> Move global_qtest assignment inside qtest_init().  Not pretty but let's
>>>> >> face it - the signal handler dependeds on global state.
>>>> >>
>>>> >> Reported-by: Marcel Apfelbaum <marcel.a@redhat.com>
>>>> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>> >> ---
>>>> >>  tests/libqtest.c | 3 ++-
>>>> >>  tests/libqtest.h | 4 +---
>>>> >>  2 files changed, 3 insertions(+), 4 deletions(-)
>>>> >>
>>>> >> diff --git a/tests/libqtest.c b/tests/libqtest.c
>>>> >> index c9e78aa..f387662 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);
>>>> >>
>>>> >> -    s = g_malloc(sizeof(*s));
>>>> >> +    global_qtest = 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,6 +181,7 @@ 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 9deebdc..7e23a4e 100644
>>>> >> --- a/tests/libqtest.h
>>>> >> +++ b/tests/libqtest.h
>>>> >> @@ -335,8 +335,7 @@ void qtest_add_func(const char *str, void (*fn));
>>>> >>   */
>>>> >>  static inline QTestState *qtest_start(const char *args)
>>>> >>  {
>>>> >> -    global_qtest = qtest_init(args);
>>>> >> -    return global_qtest;
>>>> >> +    return qtest_init(args);
>>>> >>  }
>>>> >>
>>>> >>  /**
>>>> >> @@ -347,7 +346,6 @@ static inline QTestState *qtest_start(const char *args)
>>>> >>  static inline void qtest_end(void)
>>>> >>  {
>>>> >>      qtest_quit(global_qtest);
>>>> >> -    global_qtest = NULL;
>>>> >>  }
>>>> >>
>>>> >>  /**
>>>> >
>>>> > Before this patch, the libqtest API could theoretically support multiple
>>>> > simultaneous instances of QTestState.  This patch kills that option,
>>>> > doesn't it?
>>>>
>>>> Ouch, I thought I had looked out for that...
>>>>
>>>> >
>>>> > If yes: fine with me, we don't need it anyway.
>>>>
>>>> We do. Migration and ivshmem are examples that need two machines - might
>>>> explain why my ivshmem-test was behaving unexpectedly.
>>>>
>>>> Apart from reverting, what are our options?
>>> The problem is in 'kill_qemu' function, which is called from
>>> SIGABRT signal handler.  The later needs to know the QTestState
>>> in order to kill the QEMU process.
>>>
>>> Without this patch, kill_qemu will cause a segfault because of:
>>> static void kill_qemu(QTestState *s)
>>> {
>>>     if (s->qemu_pid != -1) {
>>> ...
>>> s can be NULL if there is an assert in qtest_init.
>>>
>>> I suppose we can find a different approach, like keeping
>>> the qemu_pid(s) in another statically defined struct.
>>
>> We can keep a global linked list of QEMU pids.
>
> What about a global list of QTestState?

Yes, that's exactly what I ended up implementing.  Sending patch.

Stefan