Re: Patch to add g_child_watch_source

[Jonathan Blandford <jrb redhat com>]

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

> Thus spake Jonathan Blandford:
> > 
> > Comments are appreciated,
> 
> (I should've sent this earlier - sorry)

Thanks for the comments!

> > +static void
> > +g_child_watch_signal_handler (int signum)
> > +{
> > +  child_watch_count ++;
> > +  if (child_watch_init_state == CHILD_WATCH_INITIALIZED_THREADED)
> > +    {
> > +      write (child_watch_wake_up_pipe[1], "B", 1);
> > +    }
> > +  else
> > +    {
> > +      /* We count on the signal interrupting the poll in the same thread.
> > +       */
> > +    }
> > +}
> 
> You probably want to use the same pipe trick even in the single-threaded
> case, otherwise you won't notice the signal when it arrives between calls
> to poll.

Argh, good catch!  I didn't think of that.  That will complicate the
code a bit.  I'll see if I can get a new patch then.

> I don't think I understand what you're trying to protect with
> child_watch_count...

If child_watch_count gets incremented in between the waitpid and setting
child_watch_source->child, I could miss the child exiting.  So I make a
copy of child_watch_count to use in check_for_child_exited to keep it
consistent throughout.  I can make the comment more obvious.

> There's also some cruft left in child_watch_helper_thread , where you
> decided to use read instead of poll.

Yup.  I changed my mind.  I killed the cruft.  I also noticed this
little gem while doing so:
      G_UNLOCK (main_context_list);
      for (list = main_context_list; list; list = list->next)
        /* ... */
      G_LOCK (main_context_list);
Whoops...

Thanks a lot for the comments,
-Jonathan