Re: oaf waitpid fix

[Maciej Stachowiak <mjs eazel com>]

Dan Winship <danw helixcode com> writes:

> > > I'm not sure why I suddenly need this (some gnome-vfs thing going on
> > > that I haven't figured out yet)
> 
> Turns out to be the fact that if you call gnome_vfs_init(), your app
> suddenly has a SIGCHLD handler.

Wheeeee. Libraries setting signal handlers. Yum. But I suppose we must
handle the case of a user signal handler as well.
 
> > but it's correct anyway.
> > 
> > Looks good, apply.
> 
> But here's an even better patch. :)
> 

Hmmm; is it bad if some app or library SIGCHLD handler wait()s on this
child instead of us? It would certainly be wrong for any such signal
handler to assume when it wait()ed it would only get processes it knew
about. Then the waitpid code below would just get ECHILD. Would that
be bad?

I guess your patch looks OK though.

 - Maciej


> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/gnome/oaf/ChangeLog,v
> retrieving revision 1.87
> diff -u -r1.87 ChangeLog
> --- ChangeLog	2000/09/15 08:47:06	1.87
> +++ ChangeLog	2000/09/18 19:43:48
> @@ -1,3 +1,9 @@
> +2000-09-17  Dan Winship  <danw helixcode com>
> +
> +	* liboaf/oaf-registration.c (oaf_server_by_forking): deal with
> +	waitpid being interrupted and close race condition with SIGCHLD
> +	handler.
> +
>  2000-09-15  Jesus Bravo Alvarez  <jba pobox com>
>  
>  	* configure.in: Added Galician (gl) to ALL_LINGUAS
> Index: liboaf/oaf-registration.c
> ===================================================================
> RCS file: /cvs/gnome/oaf/liboaf/oaf-registration.c,v
> retrieving revision 1.17
> diff -u -r1.17 oaf-registration.c
> --- liboaf/oaf-registration.c	2000/07/26 01:01:50	1.17
> +++ liboaf/oaf-registration.c	2000/09/18 19:43:48
> @@ -288,13 +288,20 @@
>          int status;
>          guint watchid;
>          struct sigaction sa;
> +        sigset_t mask, omask;
>                  
>       	pipe (iopipes);
>  
> +        /* Block SIGCHLD so no one else can wait() on the child before us. */
> +        sigemptyset (&mask);
> +        sigaddset (&mask, SIGCHLD);
> +        sigprocmask (SIG_BLOCK, &mask, &omask);
> +
>  	/* fork & get the IOR from the magic pipe */
>  	childpid = fork ();
>  
>  	if (childpid < 0) {
> +                sigprocmask (SIG_SETMASK, &omask, NULL);
>  		errval = OAF_GeneralError__alloc ();
>  		errval->description = CORBA_string_dup (_("Couldn't fork a new process"));
>  
> @@ -304,7 +311,10 @@
>  	}
>  
>  	if (childpid) {
> -		waitpid (childpid, &status, 0);	/* de-zombify */
> +                /* de-zombify */
> +                while (waitpid (childpid, &status, 0) == -1 && errno == EINTR)
> +                        ;
> +                sigprocmask (SIG_SETMASK, &omask, NULL);
>                  
>  		if (!WIFEXITED (status)) {
>  			OAF_GeneralError *errval;