From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56908) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFgoS-0007x2-Fw for qemu-devel@nongnu.org; Tue, 18 Feb 2014 04:18:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFgoJ-0000wv-2C for qemu-devel@nongnu.org; Tue, 18 Feb 2014 04:18:00 -0500 Received: from mail-we0-x232.google.com ([2a00:1450:400c:c03::232]:45125) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFgoI-0000wj-S1 for qemu-devel@nongnu.org; Tue, 18 Feb 2014 04:17:50 -0500 Received: by mail-we0-f178.google.com with SMTP id q59so11540604wes.9 for ; Tue, 18 Feb 2014 01:17:49 -0800 (PST) Date: Tue, 18 Feb 2014 10:17:46 +0100 From: Stefan Hajnoczi Message-ID: <20140218091746.GC32585@stefanha-thinkpad.redhat.com> References: <1392651898-16749-1-git-send-email-stefanha@redhat.com> <1392651898-16749-4-git-send-email-stefanha@redhat.com> <878ut9u1k4.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <878ut9u1k4.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Peter Maydell , Andreas Faerber , qemu-devel@nongnu.org, Stefan Hajnoczi , Anthony Liguori On Mon, Feb 17, 2014 at 05:49:31PM +0100, Markus Armbruster wrote: > Stefan Hajnoczi writes: > > + /* Catch SIGABRT to clean up on g_assert() failure */ > > + sigact = (struct sigaction){ > > + .sa_handler = sigabrt_handler, > > + .sa_flags = SA_RESETHAND, > > + }; > > Assumes zero-initialization has the same effect as > sigemptyset(&sigact.sa_mask). Quoting POSIX: > > The implementation of the sigemptyset() (or sigfillset()) function > could quite trivially clear (or set) all the bits in the signal set. > Alternatively, it would be reasonable to initialize part of the > structure, such as a version field, to permit binary-compatibility > between releases where the size of the set varies. For such > reasons, either sigemptyset() or sigfillset() must be called prior > to any other use of the signal set, even if such use is read-only > (for example, as an argument to sigpending()). > > Looks like you better sigemptyset() here, for maximum portability. Okay, will do that. > > + sigaction(SIGABRT, &sigact, &s->sigact_old); > > > > socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid()); > > qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid()); > > @@ -150,13 +167,19 @@ void qtest_quit(QTestState *s) > > { > > int status; > > > > + sigaction(SIGABRT, &s->sigact_old, NULL); > > + > > Can this ever restore the action to anything but the default action? This code is in a "library" so I don't want to make assumptions about the callers. Keeping sigact_old around is literally 1 line of code so I think it's worth it. > > if (s->qemu_pid != -1) { > > kill(s->qemu_pid, SIGTERM); > > waitpid(s->qemu_pid, &status, 0); > > } > > > > - close(s->fd); > > - close(s->qmp_fd); > > + if (s->fd != -1) { > > + close(s->fd); > > + } > > + if (s->qmp_fd != -1) { > > + close(s->qmp_fd); > > + } > > I generally don't bother to avoid close(-1). When I drive on the highway I stay on the lanes but I guess I could just slide along the side barriers :). It's a style issue but close(-1) annoys me in strace so I try to avoid doing it.