Re: [Qemu-devel] [PATCH 2/2] qemu-ga: Add the guest-suspend command

[Jamie Lokier]

Eric Blake wrote:
> On 01/13/2012 12:15 PM, Luiz Capitulino wrote:
> > This might look complex, but the final code is quite simple. The
> > purpose of that approach is to allow qemu-ga to reap its children
> > (semi-)automatically from its SIGCHLD handler.
> 
> Yes, given your desire for the top-level qemu-ga signal handler to be
> simple, I can see why you did a double fork, so that the intermediate
> child can change the SIGCHLD behavior and actually do a blocking wait in
> the case where status should not be ignored.

An alternative is for SIGCHLD to write a byte to a non-blocking pipe
and do nothing else.  A main loop outside signal context reads from
the pipe, and on each read triggers a subloop of non-blocking
waitpid() getting child statuses until there are no more.  Because
it's outside signal context, it's safe to do anything with the child
statuses.

(A long time ago, on other unixes, this wasn't possible because
SIGCHLD would be retriggered until wait(), but it's not relevant on
anything modern.)

> > +            execlp(pmutils_bin, pmutils_bin, arg, NULL);
> 
> Do we really want to be relying on a PATH lookup, or should we be using
> an absolute path in pmutils_bin?

Since you mention async-signal-safety, execlp() isn't
async-signal-safe!  Last time I checked, in Glibc execlp() could call
malloc().  Also reading PATH looks at the environment, which isn't
always thread-safe either, depending on what else is going on.

I'm not sure if it's relevant to the this code, but on Glibc fork() is
not async-signal-safe and has been known to crash in signal handlers.
This is why fork() was removed from SUS async-signal-safe functions.

> I didn't check whether slog() is async-signal safe (probably not, since
> even snprintf() is not async-signal safe, and you are passing a printf
> style format string).  But strerror() is not, so you shouldn't be using
> it in the child if qemu-ga is multithreaded.

In general, why is multithreadedness relevant to async-signal-safety here?

Thanks,
-- Jamie