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

["Stephen R. van den Berg" <srb@cuci.nl>]

Junio C Hamano wrote:
>"Stephen R. van den Berg" <srb@cuci.nl> writes:
>>>> -	signal(SIGCHLD, SIG_DFL);
>>>> +	child_handler(0);

>>>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);

>> In return I ask: why?

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

Well, even if robustness and "the principle of least surprise" are
the prime concerns, my change could still be considered worthwhile, but
it depends on your viewpoint.

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

Yes.

>	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().

Well, not quite, that is part of the argument, the other parts are
implicit.

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

Renaming the function could do it.

>  Or are you gaining something by not
>explicitly calling signal() for the first time?  I may be missing some
>benefit for doing so.

Well, strictly speaking the benefits you're overlooking is:

 It centralises the spot where the systemcall is made to arm the
 handler.  This means that if the setup needed to arm the handler
 ever becomes more complicated in future OSes, it only needs to be
 updated in one place.  This is a direct maintainability benefit.

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

Well, I see two solutions which increase maintainability:

Solution A:
==================================
void child_handler(int signo) {
  signal(SIGCHLD, child_handler);	/* rearm always for portability */
}

main() {
  ...
  signal(SIGCHLD, child_handler);
  ...
}
==================================

Solution B:
==================================
void setup_child_handler(int signo) {
  signal(SIGCHLD, setup_child_handler);	/* rearm always for portability */
}

main() {
  ...
  setup_child_handler(0);
  ...
}
==================================

Solution C:
==================================
void setup_child_handler(void);

void child_handler(int signo) {
  setup_child_handler();		/* rearm always for portability */
}

void setup_child_handler(void) {
  signal(SIGCHLD, child_handler);
}

main() {
  ...
  setup_child_handler();
  ...
}
==================================

Solution A is what you propose, but which I find less appealing because
any future magic to actually setup the handler needs to be maintained
and updated in two places.

Solution C is what follows your train of thought better, because it
future-proofs the setup as well as the handler.

Solution B is what I consider most elegant and maintainable, because at
this point in time I cannot imagine what extra handling would be
required inside the handler which would require a setup as complicated
as solution C; so in order to keep it as simple as possible and
eliminate forward declarations and minimise systemcalls I suggest we
pick solution B until the need for solution C ever arises (I don't
think it ever will).

But, in any case, you're the maintainer here, not I, so it's your call.
I vote for B, but just tell me which solution you prefer and I'll adapt
the code?
-- 
Sincerely,
           Stephen R. van den Berg.

"Hold still, while I inject you with SQL."