Re: [RFC PATCH] daemon: add a self pipe to trigger reaping of children

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

Hi Carlo

On 28/06/2025 07:19, Carlo Marcelo Arenas Belón wrote:
> On Fri, Jun 27, 2025 at 09:38:36AM -0800, Phillip Wood wrote:
>>
>> On 26/06/2025 19:24, Carlo Marcelo Arenas Belón wrote:
>>>
>>> diff --git a/daemon.c b/daemon.c
>>> index d1be61fd57..d3b9421575 100644
>>> --- a/daemon.c
>>> +++ b/daemon.c
>>> @@ -912,14 +912,17 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
>>>    		add_child(&cld, addr, addrlen);
>>>    }
>>> -static void child_handler(int signo UNUSED)
>>> +int poll_pipe[2] = { -1, -1 };
>> Maybe call this signal_pipe? I'm not sure what poll_pipe means.
>>
>>> +
>>> +static void child_handler(int signo)
>>>    {
>>>    	/*
>>> -	 * Otherwise empty handler because systemcalls will get interrupted
>>> -	 * upon signal receipt
>>>    	 * SysV needs the handler to be rearmed
>>>    	 */
>>>    	signal(SIGCHLD, child_handler);
>>
>> I think from Chris' email that it is conventional to do this at the end of
>> the handler.
> 
> It really depends on which flags the signal was expected to use.  For maximum
> portability it would seem better to have it at the beginning to minimize the
> inherent race condition from when SA_RESETHAND is on effect (which was more
> of a SysV signal() tradition).
> 
> Will move it to the end, regardless, as it won't be needed once the call is
> setup using sigaction().
> 
>> As I said above we could add an additional commit that moves
>> this into the event loop to fix the infinite recursion on AIX.
> 
> Not sure I understant what you mean by this.

Delete the call to signal() from child_handler() and move it to the 
event loop so it is called just before poll()

> I think Chris made clear that
> the AIX issue comes from the handler being expected to do the wait() calls,

The issue is calling signal() without calling wait() first. By removing 
signal() from the signal handler there can be no recursion.

>>> +
>>> +	if (poll_pipe[1] >= 0)
>>> +		write(poll_pipe[1], &signo, 1);
>>
>> write() might fail so we should save errno around it. Conventionally one
>> would re-try on EINTR as well though in this case the most likely reason for
>> that is another child exiting which means the pipe would be written to
>> anyway.
> 
> EINTR shouldn't be possible here from SIGCHLD, because that signal is
> blocked here, 

Hmm I'm not sure that is guaranteed by POSIX though it is true with BSD 
or SysV semantics I think. From a maintainability point of view we could 
see EINTR here if we added a handler for another signal in the future so 
I think we should make sure it is handled correctly.

I wrote it this way because it was only meant to be used as
> a fallback to the EINTR being triggered in poll() and so it wouldn't matter
> if it was lost (for example because of a SIGPIPE).

I'm not convinced about making it a fallback - to me that just means 
we'll never notice if it is broken.

> Once we move to sigaction, SIGPIPE could be added to the mask for this
> handler, but that code can't be implemented on the current base.

I'm not sure I understand. The signal mask supplied to sigaction just 
blocks those signals while the handler is running, they're still 
delivered afterwards so we'd still receive SIGPIPE if the read end was 
closed. We could ignore SIGPIPE so that we get an error from write() 
instead but I'm not sure it is worth worrying about as if the read end 
of the pipe has been closed we have a bug.

>>>    }
>>>    static int set_reuse_addr(int sockfd)
>>> @@ -1121,20 +1124,43 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
>>>    static int service_loop(struct socketlist *socklist)
>>>    {
>>>    	struct pollfd *pfd;
>>> +	unsigned long nfds = 1 + socklist->nr;
>>> +
>>> +	ALLOC_ARRAY(pfd, nfds);
>>> +	if (!pipe(poll_pipe)) {
>>
>> If we cannot create a pipe here then things have gone pretty badly wrong and
>> I think it is unlikely we're going to be able to accept incoming connections
>> so it would be best to die().
> 
> True, but since this is "optional", there is no harm on letting this continue
> even in failure.

Except that we'd never know if this code was broken. We don't expect it 
to fail so why make it optional?

Thanks

Phillip