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
