From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:47228)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@redhat.com>) id 1WO5Dp-00023A-Bl
	for qemu-devel@nongnu.org; Thu, 13 Mar 2014 08:58:54 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@redhat.com>) id 1WO5Dj-000673-Bs
	for qemu-devel@nongnu.org; Thu, 13 Mar 2014 08:58:53 -0400
Received: from mx1.redhat.com ([209.132.183.28]:24333)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@redhat.com>) id 1WO5Dj-00065T-4J
	for qemu-devel@nongnu.org; Thu, 13 Mar 2014 08:58:47 -0400
Date: Thu, 13 Mar 2014 13:58:43 +0100
From: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20140313125843.GB15090@stefanha-thinkpad.redhat.com>
References: <1394703694-3281-1-git-send-email-stefanha@redhat.com>
	<1394703694-3281-3-git-send-email-stefanha@redhat.com>
	<1394708825.3981.99.camel@localhost.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1394708825.3981.99.camel@localhost.localdomain>
Subject: Re: [Qemu-devel] [PATCH 2/2] qtest: fix crash if SIGABRT during
	qtest_init()
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Marcel Apfelbaum <marcel.a@redhat.com>
Cc: qemu-devel@nongnu.org, Anthony Liguori <aliguori@amazon.com>, Andreas Faerber <afaerber@suse.de>

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