From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35900) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bK6mi-0000vN-E6 for qemu-devel@nongnu.org; Mon, 04 Jul 2016 12:31:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bK6md-0007fW-NY for qemu-devel@nongnu.org; Mon, 04 Jul 2016 12:31:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50536) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bK6md-0007fS-Fx for qemu-devel@nongnu.org; Mon, 04 Jul 2016 12:31:43 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D6DC61176D9 for ; Mon, 4 Jul 2016 16:31:42 +0000 (UTC) Date: Mon, 4 Jul 2016 17:31:39 +0100 From: "Daniel P. Berrange" Message-ID: <20160704163139.GM3763@redhat.com> Reply-To: "Daniel P. Berrange" References: <20160704153823.16879-1-marcandre.lureau@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160704153823.16879-1-marcandre.lureau@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] char: do not use atexit cleanup handler List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: marcandre.lureau@redhat.com Cc: qemu-devel@nongnu.org, pbonzini@redhat.com On Mon, Jul 04, 2016 at 05:38:23PM +0200, marcandre.lureau@redhat.com wro= te: > From: Marc-Andr=C3=A9 Lureau >=20 > It turns out qemu is calling exit() in various places from various > threads without taking much care of resources state. The atexit() > cleanup handlers cannot easily destroy resources that are in use (by > the same thread or other). [snip] > Instead of using a atexit() handler, only run the chardev cleanup as > initially proposed at the end of main(), where there are less chances > (hic) of conflicts or other races. This doesn't really seem all that much safer. There's still plenty of chance that threads are running in the background at the end of the main() method, so plenty of scope for the qemu_chr_cleanup() call to cause threads to segv by destroying the chardevs they're using behind their back. IIUC, the original intent here was that we call unlink() on the UNIX socket paths when QEMU exits. Surely we can come up with a way to that, and only that, upon exit, without actually having to free the chardev memory with all the risks that entails. eg, have a qemu_chr_close() method that closes & cleans up resources, separately from actually free'ing the Chardev struct with all the risk of crashing concurrent threads that entails. > Signed-off-by: Marc-Andr=C3=A9 Lureau > Reported-by: Andrew Jones > --- > include/sysemu/char.h | 7 +++++++ > qemu-char.c | 4 +--- > vl.c | 1 + > 3 files changed, 9 insertions(+), 3 deletions(-) >=20 > diff --git a/include/sysemu/char.h b/include/sysemu/char.h > index 57df10a..0ea9eac 100644 > --- a/include/sysemu/char.h > +++ b/include/sysemu/char.h > @@ -151,6 +151,13 @@ CharDriverState *qemu_chr_new(const char *label, c= onst char *filename, > */ > void qemu_chr_disconnect(CharDriverState *chr); > =20 > +/** > + * @qemu_chr_cleanup: > + * > + * Delete all chardevs (when leaving qemu) > + */ > +void qemu_chr_cleanup(void); > + > /** > * @qemu_chr_new_noreplay: > * > diff --git a/qemu-char.c b/qemu-char.c > index b73969d..a542192 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -4549,7 +4549,7 @@ void qmp_chardev_remove(const char *id, Error **e= rrp) > qemu_chr_delete(chr); > } > =20 > -static void qemu_chr_cleanup(void) > +void qemu_chr_cleanup(void) > { > CharDriverState *chr, *tmp; > =20 > @@ -4604,8 +4604,6 @@ static void register_types(void) > * is specified > */ > qemu_add_machine_init_done_notifier(&muxes_realize_notify); > - > - atexit(qemu_chr_cleanup); > } > =20 > type_init(register_types); > diff --git a/vl.c b/vl.c > index 9bb7f4c..d0b9ff9 100644 > --- a/vl.c > +++ b/vl.c > @@ -4596,6 +4596,7 @@ int main(int argc, char **argv, char **envp) > #ifdef CONFIG_TPM > tpm_cleanup(); > #endif > + qemu_chr_cleanup(); > =20 > return 0; Regards, Daniel --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://autobuild.org -o- http://search.cpan.org/~danberr= / :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vn= c :|