From: Paul Tarjan Date: Wed, 04 Mar 2026 18:17:53 GMT Subject: Re: [PATCH v7 10/10] fsmonitor: close inherited file descriptors and detach in daemon Message-ID: <20260304181753.25787-1-github@paulisageek.com> In-Reply-To: On Tue, Mar 4, 2026, Patrick Steinhardt wrote: > Hm. We already have related logic in `daemonize()`. Should we maybe > reuse that function, and potentially expand it to handle closing all FDs > up to the maximum file descriptor? daemonize() does a double-fork with setsid, which is the classic Unix daemon pattern. But fsmonitor uses start_bg_command(), which forks the daemon and waits for it to signal readiness over IPC. If we called daemonize() inside the child, start_bg_command() would lose track of the PID. So instead we just use setsid() to detach from the terminal, and close_fd_above_stderr in run-command to close inherited FDs before exec. > This feels like a "while-at-it" change to me. Should it maybe be moved > into a separate commit? Done. Split the 30-second stop timeout into its own commit (patch 10, "fsmonitor: add timeout to daemon stop command"). > Might make sense to reorder commits a bit so that the fix comes first. > In that case we wouldn't ever have to introduce the timeouts in the > first place. Done. Reordered in v8 so run-command and daemon detach come before the test commit. The meson timeout never appears now. > This change feels unrelated and is not mentioned in the commit message. --start-timeout=10 is now in the tests commit (patch 11) and documented in that commit message. > I thought with our call to setsid() we're not part of the same process > group anymore. So why is this change here still needed? setsid() runs inside the daemon after it's already been forked. The set -m in the test is about the shell putting `git pull &` into its own process group. Without it, the background job inherits the test shell's pgid and `wait` stalls. Moved to the tests commit (patch 11) with a note in the commit message explaining why it's still needed.