From: Daan De Meyer Date: Wed, 19 Nov 2025 13:28:49 +0000 (+0100) Subject: pidref: Add pidref_wait_for_terminate_full() X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e74b571004661ff39fbcbddfe0cbf36d2fda0046;p=thirdparty%2Fsystemd.git pidref: Add pidref_wait_for_terminate_full() We can use this to replace wait_for_terminate_with_timeout(). Because we poll a pidfd, we don't need to block SIGCHLD anymore in umount.c. --- diff --git a/src/basic/pidref.c b/src/basic/pidref.c index 2d19dea60e8..de92d2fffbe 100644 --- a/src/basic/pidref.c +++ b/src/basic/pidref.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include #include #include @@ -8,12 +9,14 @@ #include "fd-util.h" #include "format-util.h" #include "hash-funcs.h" +#include "io-util.h" #include "log.h" #include "parse-util.h" #include "pidfd-util.h" #include "pidref.h" #include "process-util.h" #include "siphash24.h" +#include "time-util.h" int pidref_acquire_pidfd_id(PidRef *pidref) { int r; @@ -449,7 +452,8 @@ bool pidref_is_self(PidRef *pidref) { return pidref->fd_id == self_id; } -int pidref_wait(PidRef *pidref, siginfo_t *ret, int options) { +int pidref_wait_for_terminate_full(PidRef *pidref, usec_t timeout, siginfo_t *ret) { + siginfo_t si = {}; int r; if (!pidref_is_set(pidref)) @@ -461,28 +465,42 @@ int pidref_wait(PidRef *pidref, siginfo_t *ret, int options) { if (pidref->pid == 1 || pidref_is_self(pidref)) return -ECHILD; - siginfo_t si = {}; - if (pidref->fd >= 0) - r = RET_NERRNO(waitid(P_PIDFD, pidref->fd, &si, options)); - else - r = RET_NERRNO(waitid(P_PID, pidref->pid, &si, options)); - if (r < 0) - return r; - - if (ret) - *ret = si; - - return 0; -} + if (timeout != USEC_INFINITY && pidref->fd < 0) + return -ENOMEDIUM; -int pidref_wait_for_terminate(PidRef *pidref, siginfo_t *ret) { - int r; + usec_t ts = timeout == USEC_INFINITY ? USEC_INFINITY : now(CLOCK_MONOTONIC) + timeout; for (;;) { - r = pidref_wait(pidref, ret, WEXITED); - if (r != -EINTR) + if (timeout != USEC_INFINITY) { + usec_t left = usec_sub_unsigned(ts, now(CLOCK_MONOTONIC)); + if (left == 0) + return -ETIMEDOUT; + + r = fd_wait_for_event(pidref->fd, POLLIN, left); + if (r == 0) + return -ETIMEDOUT; + if (r == -EINTR) + continue; + if (r < 0) + return r; + } + + if (pidref->fd >= 0) + r = RET_NERRNO(waitid(P_PIDFD, pidref->fd, &si, WEXITED)); + else + r = RET_NERRNO(waitid(P_PID, pidref->pid, &si, WEXITED)); + if (r == -EINTR) + continue; + if (r < 0) return r; + + break; } + + if (ret) + *ret = si; + + return 0; } bool pidref_is_automatic(const PidRef *pidref) { diff --git a/src/basic/pidref.h b/src/basic/pidref.h index c9185afd8d8..d778d49f09a 100644 --- a/src/basic/pidref.h +++ b/src/basic/pidref.h @@ -96,8 +96,10 @@ int pidref_kill(const PidRef *pidref, int sig); int pidref_kill_and_sigcont(const PidRef *pidref, int sig); int pidref_sigqueue(const PidRef *pidref, int sig, int value); -int pidref_wait(PidRef *pidref, siginfo_t *ret, int options); -int pidref_wait_for_terminate(PidRef *pidref, siginfo_t *ret); +int pidref_wait_for_terminate_full(PidRef *pidref, usec_t timeout, siginfo_t *ret); +static inline int pidref_wait_for_terminate(PidRef *pidref, siginfo_t *ret) { + return pidref_wait_for_terminate_full(pidref, USEC_INFINITY, ret); +} static inline void pidref_done_sigkill_wait(PidRef *pidref) { if (!pidref_is_set(pidref)) @@ -108,6 +110,14 @@ static inline void pidref_done_sigkill_wait(PidRef *pidref) { pidref_done(pidref); } +static inline void pidref_done_sigkill_nowait(PidRef *pidref) { + if (!pidref_is_set(pidref)) + return; + + (void) pidref_kill(pidref, SIGKILL); + pidref_done(pidref); +} + int pidref_verify(const PidRef *pidref); #define TAKE_PIDREF(p) TAKE_GENERIC((p), PidRef, PIDREF_NULL) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index e319214f4aa..94522fb495e 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -912,69 +912,6 @@ int wait_for_terminate_and_check(const char *name, pid_t pid, WaitFlags flags) { return pidref_wait_for_terminate_and_check(name, &PIDREF_MAKE_FROM_PID(pid), flags); } -/* - * Return values: - * - * < 0 : wait_for_terminate_with_timeout() failed to get the state of the process, the process timed out, the process - * was terminated by a signal, or failed for an unknown reason. - * - * >=0 : The process terminated normally with no failures. - * - * Success is indicated by a return value of zero, a timeout is indicated by ETIMEDOUT, and all other child failure - * states are indicated by error is indicated by a non-zero value. - * - * This call assumes SIGCHLD has been blocked already, in particular before the child to wait for has been forked off - * to remain entirely race-free. - */ -int wait_for_terminate_with_timeout(pid_t pid, usec_t timeout) { - sigset_t mask; - int r; - usec_t until; - - assert_se(sigemptyset(&mask) == 0); - assert_se(sigaddset(&mask, SIGCHLD) == 0); - - /* Drop into a sigtimewait-based timeout. Waiting for the - * pid to exit. */ - until = usec_add(now(CLOCK_MONOTONIC), timeout); - for (;;) { - usec_t n; - siginfo_t status = {}; - - n = now(CLOCK_MONOTONIC); - if (n >= until) - break; - - r = RET_NERRNO(sigtimedwait(&mask, NULL, TIMESPEC_STORE(until - n))); - /* Assuming we woke due to the child exiting. */ - if (waitid(P_PID, pid, &status, WEXITED|WNOHANG) == 0) { - if (status.si_pid == pid) { - /* This is the correct child. */ - if (status.si_code == CLD_EXITED) - return status.si_status == 0 ? 0 : -EPROTO; - else - return -EPROTO; - } - } - /* Not the child, check for errors and proceed appropriately */ - if (r < 0) { - switch (r) { - case -EAGAIN: - /* Timed out, child is likely hung. */ - return -ETIMEDOUT; - case -EINTR: - /* Received a different signal and should retry */ - continue; - default: - /* Return any unexpected errors */ - return r; - } - } - } - - return -EPROTO; -} - void sigkill_wait(pid_t pid) { assert(pid > 1); diff --git a/src/basic/process-util.h b/src/basic/process-util.h index bb65f0eedc2..4b7641da6ba 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -73,8 +73,6 @@ typedef enum WaitFlags { int pidref_wait_for_terminate_and_check(const char *name, PidRef *pidref, WaitFlags flags); int wait_for_terminate_and_check(const char *name, pid_t pid, WaitFlags flags); -int wait_for_terminate_with_timeout(pid_t pid, usec_t timeout); - void sigkill_wait(pid_t pid); void sigkill_waitp(pid_t *pid); void sigterm_wait(pid_t pid); diff --git a/src/core/socket.c b/src/core/socket.c index 848cb313720..b2b21a8a6d4 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -1653,7 +1653,7 @@ static int socket_address_listen_in_cgroup( fd = receive_one_fd(pair[0], 0); /* We synchronously wait for the helper, as it shouldn't be slow */ - r = wait_for_terminate_and_check("(sd-listen)", pid.pid, WAIT_LOG_ABNORMAL); + r = pidref_wait_for_terminate_and_check("(sd-listen)", &pid, WAIT_LOG_ABNORMAL); if (r < 0) { safe_close(fd); return r; @@ -3181,7 +3181,7 @@ static int socket_accept_in_cgroup(Socket *s, SocketPort *p, int fd) { cfd = receive_one_fd(pair[0], 0); /* We synchronously wait for the helper, as it shouldn't be slow */ - r = wait_for_terminate_and_check("(sd-accept)", pid.pid, WAIT_LOG_ABNORMAL); + r = pidref_wait_for_terminate_and_check("(sd-accept)", &pid, WAIT_LOG_ABNORMAL); if (r < 0) { safe_close(cfd); return r; diff --git a/src/home/homed-home.c b/src/home/homed-home.c index cd560c9a731..92fb1d9c2d1 100644 --- a/src/home/homed-home.c +++ b/src/home/homed-home.c @@ -3299,11 +3299,14 @@ int home_wait_for_worker(Home *h) { log_info("Worker process for home %s is still running while exiting. Waiting for it to finish.", h->user_name); - r = wait_for_terminate_with_timeout(h->worker_pid.pid, 30 * USEC_PER_SEC); + siginfo_t si; + r = pidref_wait_for_terminate_full(&h->worker_pid, 30 * USEC_PER_SEC, &si); if (r == -ETIMEDOUT) log_warning_errno(r, "Waiting for worker process for home %s timed out. Ignoring.", h->user_name); else if (r < 0) - log_warning_errno(r, "Failed to wait for worker process for home %s. Ignoring.", h->user_name); + log_warning_errno(r, "Failed to wait for worker process for home %s, ignoring: %m", h->user_name); + else if (si.si_code != CLD_EXITED || si.si_status != 0) + log_warning("Worker process for home %s failed with non-zero exit status. Ignoring.", h->user_name); (void) hashmap_remove_value(h->manager->homes_by_worker_pid, &h->worker_pid, h); pidref_done(&h->worker_pid); diff --git a/src/shutdown/shutdown.c b/src/shutdown/shutdown.c index 25882970ef6..3496ad33ea8 100644 --- a/src/shutdown/shutdown.c +++ b/src/shutdown/shutdown.c @@ -228,8 +228,6 @@ int sync_with_progress(int fd) { _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; int r; - BLOCK_SIGNALS(SIGCHLD); - /* Due to the possibility of the sync operation hanging, we fork a child process and monitor * the progress. If the timeout lapses, the assumption is that the particular sync stalled. */ @@ -252,7 +250,7 @@ int sync_with_progress(int fd) { * SYNC_PROGRESS_ATTEMPTS lapse without progress being made, * we assume that the sync is stalled */ for (unsigned checks = 0; checks < SYNC_PROGRESS_ATTEMPTS; checks++) { - r = wait_for_terminate_with_timeout(pidref.pid, SYNC_TIMEOUT_USEC); + r = pidref_wait_for_terminate_full(&pidref, SYNC_TIMEOUT_USEC, /* ret= */ NULL); if (r == 0) /* Sync finished without error (sync() call itself does not return an error code) */ return 0; diff --git a/src/shutdown/umount.c b/src/shutdown/umount.c index 46f208824aa..9f061177ee3 100644 --- a/src/shutdown/umount.c +++ b/src/shutdown/umount.c @@ -24,9 +24,9 @@ #include "mount-util.h" #include "mountpoint-util.h" #include "parse-util.h" +#include "pidref.h" #include "process-util.h" #include "random-util.h" -#include "signal-util.h" #include "stat-util.h" #include "string-util.h" #include "umount.h" @@ -250,11 +250,9 @@ static void log_umount_blockers(const char *mnt) { static int remount_with_timeout(MountPoint *m, bool last_try) { _cleanup_close_pair_ int pfd[2] = EBADF_PAIR; - _cleanup_(sigkill_nowaitp) pid_t pid = 0; + _cleanup_(pidref_done_sigkill_nowait) PidRef pidref = PIDREF_NULL; int r; - BLOCK_SIGNALS(SIGCHLD); - assert(m); r = pipe2(pfd, O_CLOEXEC|O_NONBLOCK); @@ -263,10 +261,10 @@ static int remount_with_timeout(MountPoint *m, bool last_try) { /* Due to the possibility of a remount operation hanging, we fork a child process and set a * timeout. If the timeout lapses, the assumption is that the particular remount failed. */ - r = safe_fork_full("(sd-remount)", + r = pidref_safe_fork_full("(sd-remount)", NULL, pfd, ELEMENTSOF(pfd), - FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_LOG|FORK_REOPEN_LOG, &pid); + FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_LOG|FORK_REOPEN_LOG, &pidref); if (r < 0) return r; if (r == 0) { @@ -287,29 +285,37 @@ static int remount_with_timeout(MountPoint *m, bool last_try) { pfd[1] = safe_close(pfd[1]); - r = wait_for_terminate_with_timeout(pid, DEFAULT_TIMEOUT_USEC); + siginfo_t si; + r = pidref_wait_for_terminate_full(&pidref, DEFAULT_TIMEOUT_USEC, &si); if (r == -ETIMEDOUT) - log_error_errno(r, "Remounting '%s' timed out, issuing SIGKILL to PID " PID_FMT ".", m->path, pid); - else if (r == -EPROTO) { + log_error_errno(r, "Remounting '%s' timed out, issuing SIGKILL to PID " PID_FMT ".", m->path, pidref.pid); + else if (r < 0) + log_error_errno(r, "Remounting '%s' failed unexpectedly, couldn't wait for child process " PID_FMT ": %m", m->path, pidref.pid); + else if (si.si_code != CLD_EXITED || si.si_status != 0) { /* Try to read error code from child */ - if (read(pfd[0], &r, sizeof(r)) == sizeof(r)) - log_debug_errno(r, "Remounting '%s' failed abnormally, child process " PID_FMT " failed: %m", m->path, pid); - else - r = log_debug_errno(EPROTO, "Remounting '%s' failed abnormally, child process " PID_FMT " aborted or exited non-zero.", m->path, pid); - TAKE_PID(pid); /* child exited (just not as we expected) hence don't kill anymore */ - } else if (r < 0) - log_error_errno(r, "Remounting '%s' failed unexpectedly, couldn't wait for child process " PID_FMT ": %m", m->path, pid); + r = read_errno(pfd[0]); + if (r == -EIO) + r = log_debug_errno( + SYNTHETIC_ERRNO(EPROTO), + "Remounting '%s' failed abnormally, child process " PID_FMT " aborted or exited non-zero.", + m->path, pidref.pid); + else if (r < 0) + log_debug_errno( + r, + "Remounting '%s' failed abnormally, child process " PID_FMT " failed: %m", + m->path, pidref.pid); + + TAKE_PIDREF(pidref); /* child exited (just not as we expected) hence don't kill anymore */ + } return r; } static int umount_with_timeout(MountPoint *m, bool last_try) { _cleanup_close_pair_ int pfd[2] = EBADF_PAIR; - _cleanup_(sigkill_nowaitp) pid_t pid = 0; + _cleanup_(pidref_done_sigkill_nowait) PidRef pidref = PIDREF_NULL; int r; - BLOCK_SIGNALS(SIGCHLD); - assert(m); r = pipe2(pfd, O_CLOEXEC|O_NONBLOCK); @@ -318,10 +324,10 @@ static int umount_with_timeout(MountPoint *m, bool last_try) { /* Due to the possibility of a umount operation hanging, we fork a child process and set a * timeout. If the timeout lapses, the assumption is that the particular umount failed. */ - r = safe_fork_full("(sd-umount)", + r = pidref_safe_fork_full("(sd-umount)", NULL, pfd, ELEMENTSOF(pfd), - FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_LOG|FORK_REOPEN_LOG, &pid); + FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_LOG|FORK_REOPEN_LOG, &pidref); if (r < 0) return r; if (r == 0) { @@ -349,18 +355,28 @@ static int umount_with_timeout(MountPoint *m, bool last_try) { pfd[1] = safe_close(pfd[1]); - r = wait_for_terminate_with_timeout(pid, DEFAULT_TIMEOUT_USEC); + siginfo_t si; + r = pidref_wait_for_terminate_full(&pidref, DEFAULT_TIMEOUT_USEC, &si); if (r == -ETIMEDOUT) - log_error_errno(r, "Unmounting '%s' timed out, issuing SIGKILL to PID " PID_FMT ".", m->path, pid); - else if (r == -EPROTO) { + log_error_errno(r, "Unmounting '%s' timed out, issuing SIGKILL to PID " PID_FMT ".", m->path, pidref.pid); + else if (r < 0) + log_error_errno(r, "Unmounting '%s' failed unexpectedly, couldn't wait for child process " PID_FMT ": %m", m->path, pidref.pid); + else if (si.si_code != CLD_EXITED || si.si_status != 0) { /* Try to read error code from child */ - if (read(pfd[0], &r, sizeof(r)) == sizeof(r)) - log_debug_errno(r, "Unmounting '%s' failed abnormally, child process " PID_FMT " failed: %m", m->path, pid); - else - r = log_debug_errno(EPROTO, "Unmounting '%s' failed abnormally, child process " PID_FMT " aborted or exited non-zero.", m->path, pid); - TAKE_PID(pid); /* It died, but abnormally, no purpose in killing */ - } else if (r < 0) - log_error_errno(r, "Unmounting '%s' failed unexpectedly, couldn't wait for child process " PID_FMT ": %m", m->path, pid); + r = read_errno(pfd[0]); + if (r == -EIO) + r = log_debug_errno( + SYNTHETIC_ERRNO(EPROTO), + "Unmounting '%s' failed abnormally, child process " PID_FMT " aborted or exited non-zero.", + m->path, pidref.pid); + else if (r < 0) + log_debug_errno( + r, + "Unmounting '%s' failed abnormally, child process " PID_FMT " failed: %m", + m->path, pidref.pid); + + TAKE_PIDREF(pidref); /* It died, but abnormally, no purpose in killing */ + } return r; } diff --git a/src/test/test-pidref.c b/src/test/test-pidref.c index 3a5f44053ba..f682b42d46e 100644 --- a/src/test/test-pidref.c +++ b/src/test/test-pidref.c @@ -6,6 +6,7 @@ #include "process-util.h" #include "stdio-util.h" #include "tests.h" +#include "time-util.h" TEST(pidref_is_set) { ASSERT_FALSE(pidref_is_set(NULL)); @@ -246,4 +247,22 @@ TEST(pidref_is_remote) { ASSERT_ERROR(pidref_verify(&p), EREMOTE); } +TEST(pidref_wait_for_terminate_timeout) { + _cleanup_(pidref_done_sigkill_wait) PidRef pidref = PIDREF_NULL; + siginfo_t si; + + /* Test successful termination within timeout */ + ASSERT_OK(pidref_safe_fork("(test-pidref-wait-timeout)", FORK_DEATHSIG_SIGKILL|FORK_FREEZE, &pidref)); + + assert_se(pidref_kill(&pidref, SIGKILL) >= 0); + ASSERT_OK(pidref_wait_for_terminate_full(&pidref, 5 * USEC_PER_SEC, &si)); + ASSERT_EQ(si.si_signo, SIGCHLD); + + pidref_done(&pidref); + + /* Test timeout when process doesn't terminate */ + ASSERT_OK(pidref_safe_fork("(test-pidref-wait-timeout-expired)", FORK_DEATHSIG_SIGKILL|FORK_FREEZE, &pidref)); + ASSERT_ERROR(pidref_wait_for_terminate_full(&pidref, 100 * USEC_PER_MSEC, NULL), ETIMEDOUT); +} + DEFINE_TEST_MAIN(LOG_DEBUG);