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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 26, 2025, 18:04 UTC
Message-ID: <087d437f-3163-4c63-b6f5-e5d726016359@gmail.com>
In-Reply-To: <wanrtwacxrjmmpfnjwxhgdfhlo4uvnktijnc2rxdzlnkpe5r4a@3b2e2onpqakp>

Hi Carlo

On 26/06/2025 17:36, Carlo Marcelo Arenas Belón wrote:

Show 12 quoted lines

> 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