From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33683) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTARo-0006qH-U5 for qemu-devel@nongnu.org; Thu, 27 Mar 2014 09:34:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WTARe-0001vF-Qq for qemu-devel@nongnu.org; Thu, 27 Mar 2014 09:34:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43528) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTARe-0001v0-JU for qemu-devel@nongnu.org; Thu, 27 Mar 2014 09:34:10 -0400 Message-ID: <1395927271.19041.10.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Thu, 27 Mar 2014 15:34:31 +0200 In-Reply-To: <533423D2.6030402@suse.de> 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> <533423D2.6030402@suse.de> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 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: Andreas =?ISO-8859-1?Q?F=E4rber?= Cc: Anthony Liguori , Markus Armbruster , Stefan Hajnoczi , qemu-devel@nongnu.org On Thu, 2014-03-27 at 14:12 +0100, Andreas F=C3=A4rber wrote: > 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 QEM= U > >> process to avoid leaking it when the test dies. > >> > >> The global_qtest pointer used by the SIGABRT handler is currently on= ly > >> 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 le= t'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 multi= ple > > simultaneous instances of QTestState. This patch kills that option, > > doesn't it? >=20 > Ouch, I thought I had looked out for that... >=20 > >=20 > > If yes: fine with me, we don't need it anyway. >=20 > We do. Migration and ivshmem are examples that need two machines - migh= t > explain why my ivshmem-test was behaving unexpectedly. >=20 > 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 !=3D -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. =20 Thanks, Marcel =20 >=20 > Regards, > Andreas >=20 > > But shouldn't we clean > > up the API then? It typically provides a function taking a QTestStat= e > > parameter, and a wrapper that passes global_qtest. If global_qtest i= s > > the only QTestState we can have, having the former function is > > pointless. > >=20 >=20 >=20