Re: [PATCH 2/3] git-daemon: make the signal handler almost a no-op

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 14, 2008, 00:09 UTC
Message-ID: <7v1w0sjw47.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080813084331.30845.74788.stgit@aristoteles.cuci.nl>

"Stephen R. van den Berg" <srb@cuci.nl> writes:

> by exploiting the fact that systemcalls get interrupted by signals;
> and even they aren't, all zombies will be collected before the next
> accept().

Dscho may want to say something about "even they aren't..." part, after he comes back to the keyboard.

> Fix another error() -> logerror() call.

which should have been in 1/3, I suppose.

Show 9 quoted lines

> @@ -1036,10 +1034,7 @@ int main(int argc, char **argv)
>  	gid_t gid = 0;
>  	int i;
>  
> -	/* Without this we cannot rely on waitpid() to tell
> -	 * what happened to our children.
> -	 */
> -	signal(SIGCHLD, SIG_DFL);
> +	child_handler(0);

Why?

child_handler() logically is a two step process:

 * We are just informed that somebody died; let's do something about the
   corpse;

 * On some systems we need to rearm signals once they fired, so let's do
   that if necessary.

With your change, the first part happens to be almost no-op, but I do not think it justifies this hunk.

After all, we might even want to do something like:

	static void child_handler(int signo)
        {
        	if (USE_SYSV_SIGNAL_SEMANTICS)
                	signal(SIGCHLD, child_handler);
	}

and have the compiler optimize out the signal rearming with

	cc CFLAGS=-DUSE_SYSV_SIGNAL_SEMANTICS=0

on suitable platforms in the future. But you still want the initial signal set-up to happen unconditionally.

At this point, we aren't informed by the system that somebody died, and we would want to arm the signal regardless of the platform's signal semantics.

The rest of the patch looked sane, although I did not read it very carefully.

Thanks.