From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54365) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WO3TS-0001q2-AW for qemu-devel@nongnu.org; Thu, 13 Mar 2014 07:07:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WO3TJ-00015Z-CU for qemu-devel@nongnu.org; Thu, 13 Mar 2014 07:06:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:7674) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WO3TJ-00015T-2N for qemu-devel@nongnu.org; Thu, 13 Mar 2014 07:06:45 -0400 Message-ID: <1394708825.3981.99.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Thu, 13 Mar 2014 13:07:05 +0200 In-Reply-To: <1394703694-3281-3-git-send-email-stefanha@redhat.com> References: <1394703694-3281-1-git-send-email-stefanha@redhat.com> <1394703694-3281-3-git-send-email-stefanha@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] qtest: fix crash if SIGABRT during qtest_init() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel@nongnu.org, Anthony Liguori , Andreas Faerber 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 > Signed-off-by: Stefan Hajnoczi > --- > 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; > } > > /**