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

From: Stephen R. van den Berg <srb@cuci.nl>
Date: Aug 14, 2008, 00:18 UTC
Message-ID: <20080814001858.GB14939@cuci.nl>
In-Reply-To: <7v1w0sjw47.fsf@gitster.siamese.dyndns.org>

Junio C Hamano wrote:

>"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.

That should have read "even if they aren't". Nonetheless, I don't know systems where it doesn't work this way, but even if a system resisted, the problem is mitigated by the fact that we reap the children before every accept.

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

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

Sort of, yes, it was a bit messy to get it out in one piece.

>> @@ -1036,10 +1034,7 @@ int main(int argc, char **argv)
>>  	gid_t gid = 0;
>>  	int i;

Show 5 quoted lines

>> -	/* Without this we cannot rely on waitpid() to tell
>> -	 * what happened to our children.
>> -	 */
>> -	signal(SIGCHLD, SIG_DFL);
>> +	child_handler(0);

>Why?

child_handler() now does barely more than setup the signal handler, which is exactly what we want to do here.

>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

In return I ask: why? There is no particular performance reason to optimise this. So in order to keep the code simpler, it might make an extra unneeded systemcall on some systems when the signal is processed. I don't think it's worth our while to optimise this further.

-- 
Sincerely,
           Stephen R. van den Berg.

"And now for something *completely* different!"