From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57144) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTA7A-00081w-95 for qemu-devel@nongnu.org; Thu, 27 Mar 2014 09:13:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WTA72-0003MP-S2 for qemu-devel@nongnu.org; Thu, 27 Mar 2014 09:13:00 -0400 Received: from cantor2.suse.de ([195.135.220.15]:56885 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTA72-0003MD-IW for qemu-devel@nongnu.org; Thu, 27 Mar 2014 09:12:52 -0400 Message-ID: <533423D2.6030402@suse.de> Date: Thu, 27 Mar 2014 14:12:50 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1394703694-3281-1-git-send-email-stefanha@redhat.com> <1394703694-3281-3-git-send-email-stefanha@redhat.com> <87vbuzokj8.fsf@blackfin.pond.sub.org> In-Reply-To: <87vbuzokj8.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable 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: Markus Armbruster , Stefan Hajnoczi Cc: qemu-devel@nongnu.org, Anthony Liguori , Marcel Apfelbaum Am 27.03.2014 14:09, schrieb Markus Armbruster: > Reply after commit, sorry. >=20 > Stefan Hajnoczi writes: >=20 >> 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 >> 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 =3D getenv("QTEST_QEMU_BINARY"); >> g_assert(qemu_binary !=3D NULL); >> =20 >> - s =3D g_malloc(sizeof(*s)); >> + global_qtest =3D s =3D g_malloc(sizeof(*s)); >> =20 >> socket_path =3D g_strdup_printf("/tmp/qtest-%d.sock", getpid()); >> qmp_socket_path =3D 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 =3D NULL; >> =20 >> 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 =3D qtest_init(args); >> - return global_qtest; >> + return qtest_init(args); >> } >> =20 >> /** >> @@ -347,7 +346,6 @@ static inline QTestState *qtest_start(const char *= args) >> static inline void qtest_end(void) >> { >> qtest_quit(global_qtest); >> - global_qtest =3D NULL; >> } >> =20 >> /** >=20 > Before this patch, the libqtest API could theoretically support multipl= e > simultaneous instances of QTestState. This patch kills that option, > doesn't it? Ouch, I thought I had looked out for that... >=20 > 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. >=20 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg