Re: GChildWatchSource take three

["Alexis S. L. Carvalho" <alexis cecm usp br>]

Sorry I couldn't get to this earlier.

Just a few minor comments and questions - where "question" means that I
have no idea what I'm talking about, and the answer may very well be
"obviouslly not!"...


> +static sig_atomic_t child_watch_count = 1;

This one should be volatile :-)
(grepping /usr/include shows that sig_atomic_t is typedef'd to a
simple, non-volatile int)


> @@ -2328,6 +2384,16 @@ g_main_context_iterate (GMainContext *co
[...]
> +  main_context_about_to_poll = context;
>    some_ready = g_main_context_prepare (context, &max_priority); 
>    
>    while ((nfds = g_main_context_query (context, max_priority, &timeout, fds, 
> @@ -2344,7 +2410,8 @@ g_main_context_iterate (GMainContext *co
>      timeout = 0;
>    
>    g_main_context_poll (context, timeout, max_priority, fds, nfds);
> -  
> +  main_context_about_to_poll = NULL;
> +

A question:  isn't it possible for g_main_context_iterate to be called
recursively while main_context_about_to_poll is non-null (i.e. isn't it
necessary to save it's old value and restore it later)?


> +static void
> +g_child_watch_signal_handler (int signum)
> +{
[...]
> +      if (main_context_about_to_poll)
> +	{
> +	  g_main_context_wakeup_unlocked (main_context_about_to_poll);
> +	}

Another question:  besides what Owen said, when G_OS_WIN32 is #define'd,
this function calls ReleaseSemaphore - is it safe to call it from a
signal handler?  Then again, I don't know if this is also supposed to
work on Windows...


> +/* Separate thread that sits around waiting for messages from the signal
> + * handler.
> + */
> +static gpointer
> +child_watch_helper_thread (gpointer data)

It may be worth unblocking SIGCHLD here, too - signal masks are
per-thread and you only unblock SIGCHLD after launching this thread.


> +GSource *
> +g_child_watch_source_new (gint pid)
> +{
[...]
> +  /* We want to run on the first pass no matter what, to catch the case where
> +   * the child has exited before the signal handler has been installed. */
> +  child_watch_source->count = child_watch_count - 1;

This also catches the more general case where the child has already
exited by the time we got here.


Alexis