Re: Patch to add g_child_watch_source

[Jonathan Blandford <jrb redhat com>]

"Alexis S. L. Carvalho" <alexis cecm usp br> writes:

> Actually I hadn't understood why you were using child_watch_count in the
> first place.  Now I see that you're trying to avoid calling waitpid when
> there was no SIGCHLD.

Yeah.  The trivial implementation of the ChildWatchSource is to have
each source call waitpid each iteration of the idle.  We can do better
than that, though.

> Here are two other races:
> 
> 	parent runs child
> 	child exits; SIGCHLD is delivered to the parent
> 	parent calls g_child_watch_source_new
> 
> You'll end up calling waitpid for this child only when another SIGCHLD
> arrives.  One way to handle this race is to call waitpid in
> g_child_watch_source_new.

Good point.  I wonder if we need to call waitpid anyway the first time
we're in the loop, though.  Testing for the child right after the signal
handler has been installed seems like it would work here.

> The other race is the same one, but for the first child:  the signal
> handler is still not installed.  Usually, this isn't a problem - the
> SIGCHLD will be ignored and you'll be able to call waitpid.  But if the
> application had a signal handler installed or had SIGCHLD blocked, you
> may have some problems.  You may argue that an application that has a
> handler for SIGCHLD and uses GChildWatch sources is broken, but since
> signal masks are inherited through fork/exec I think you should unblock
> SIGCHLD (with sigprocmask).

I will argue that, but you're right, I should unblock SIGCHLD.

> The easiest way to deal with this is probably an initialization function
> that has to be called before forking the first child.

I'm not sure how much of glib is supposed to survive a fork() -- gtk+
certainly doesn't.  Just making sure that glib is set up correctly for a
signal handler should be fine.

> Also, you may want to save errno before the call to write in the signal
> handler and restore it later, just in case.

Good idea.

> Finally, it would be nice if the documentation mentioned that since
> you're dealing with signal handlers, some system calls may fail, setting
> errno to EINTR.

Good point.  I'll add that too.  I'm still trying to figure out how to
handle the last race condition you posted, though, so it might be a
little while before I post a new patch.

Thanks,
-Jonathan