Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Markus Armbruster]

Stefan Hajnoczi <address@hidden> writes:

> On Mon, Feb 17, 2014 at 05:49:31PM +0100, Markus Armbruster wrote:
>> Stefan Hajnoczi <address@hidden> 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).