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@xxxxxxxxx> 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@xxxxxxxxx)
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
