| Message ID | 1394703694-3281-3-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
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; > } > > /**
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
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
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.
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. >
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. > > > >
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
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
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?
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
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; } /**
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(-)