Re: [PATCH v3 3/4] daemon: use sigaction() to install child_handler()

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Carlo

On 26/06/2025 17:36, Carlo Marcelo Arenas Belón wrote:
> On Thu, Jun 26, 2025 at 08:33:29AM -0800, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>
>> Thanks for a suggestion.  I really like the "everybody in these code
>> paths are prepared to receive and handle EINTR just fine, so we do
>> not have to do the SA_RESTART" observation very much.
> 
> Except that is unlikely to be true, as the code has changed a lot on
> those 20 years (including that it now uses run_command) and that we
> had been effectively using SA_RESTART under the covers for most of
> that time because signal() behaviour changed. An obvious bug:
> (ex: 20250626161038.85966-1-carenas@gmail.com)

I don't think the syscall handling has changed much since 695605b508 
(git-daemon: Simplify dead-children reaping logic, 2008-08-14) when we 
started relying on poll() returning EINTR to collect children that exit 
while we are waiting for a new connection. In any case my comment was 
based on reading the code. You're right that we started using 
run_command() after 695605b508 but when I read the code in run-command.c 
it looked to me like it is pretty careful in the way it handles signals 
and EINTR - what part of the code are you concerned about? As git 
supports operating systems that do not provide SA_RESTART we should to 
make sure we handle EINTR correctly in this code anyway. As far as the 
patch you link to goes, it may not have been handling EINTR as 
efficiently as you'd like but it would still accept the client on the 
next iteration of the loop which would happen immediately because the 
connection would be pending when we call poll().

> I think using SA_RESTART by default might be safer,

The counter argument to this is that it is masking bugs that happen on 
platforms without SA_RESTART.

Best Wishes

Phillip