Stefan Hajnoczi <stefa...@gmail.com> writes: > On Mon, Feb 17, 2014 at 05:49:31PM +0100, Markus Armbruster wrote: >> Stefan Hajnoczi <stefa...@redhat.com> 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.
For me, it's in the same category as free(NULL).