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, 01:09 UTC
Message-ID: <7v63q4ieqw.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080814001858.GB14939@cuci.nl>

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

Show 30 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() 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?

Please read the part you omitted from your quote again.

I agree it would be very meaningless change as an optimization, but I am concerned more about robustness and what makes sense.

Do you agree that "child_handler()" is a signal handler to handle SIGCHLD, and such a signal handler conceptually consists of two parts? i.e.

	static void child_handler()
        {
        	reap_dead_children();
                rearm_signal_as_needed();
	}

Your argument is it is Ok to call this function when you are arming the signal for the first time, because reap_dead_children() happens to be empty, and your rearm_signal_as_needed() happens to be the same as arm_signal_always().

Yes, it happens to be _Ok_ now. But is it an improvement in the longer term? I do not think so.

I do not see why you think it is better to rely on these two assumptions than being explicit and say "we set up the signal for the first time always on any platform", especially when the latter is much more direct way to say what your intention is. Or are you gaining something by not explicitly calling signal() for the first time? I may be missing some benefit for doing so.

It is a trade-off between that some benefit I am not seeing, and downside that your version can be broken more easily by future changes to child_handler(), because you are assuming more about what it happens to do currently.

That's the kind of thing maintainers worry more about.