From: Daan De Meyer Date: Sat, 20 Dec 2025 21:10:02 +0000 (+0100) Subject: Follow ups for #39806 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f9708c53aa28bd2ba8273c5c132c1e8a83f2056e;p=thirdparty%2Fsystemd.git Follow ups for #39806 We drop the optimization for verify_sigchld as the check differs based on the options passed and the extra syscalls are certainly not going to matter compared to the cost of forking a child process. Fixes #40166 --- diff --git a/src/home/homework-luks.c b/src/home/homework-luks.c index 8ed45554511..10ed181fcf5 100644 --- a/src/home/homework-luks.c +++ b/src/home/homework-luks.c @@ -7,7 +7,6 @@ #include #include #include -#include "pidref.h" #if HAVE_VALGRIND_MEMCHECK_H #include #endif @@ -52,6 +51,7 @@ #include "openssl-util.h" #include "parse-util.h" #include "path-util.h" +#include "pidref.h" #include "process-util.h" #include "random-util.h" #include "reread-partition-table.h" @@ -2606,7 +2606,7 @@ static int ext4_offline_resize_fs( _cleanup_free_ char *size_str = NULL; bool re_open = false, re_mount = false; - _cleanup_(pidref_done) PidRef resize_pidref = PIDREF_NULL, fsck_pidref = PIDREF_NULL; + _cleanup_(pidref_done) PidRef fsck_pidref = PIDREF_NULL; int r, exit_status; assert(setup); @@ -2665,7 +2665,7 @@ static int ext4_offline_resize_fs( r = pidref_safe_fork( "(e2resize)", FORK_RESET_SIGNALS|FORK_RLIMIT_NOFILE_SAFE|FORK_DEATHSIG_SIGTERM|FORK_LOG|FORK_WAIT|FORK_STDOUT_TO_STDERR|FORK_CLOSE_ALL_FDS, - &resize_pidref); + /* ret_pid= */ NULL); if (r < 0) return r; if (r == 0) { diff --git a/src/libsystemd/sd-event/sd-event.c b/src/libsystemd/sd-event/sd-event.c index 8c549eb22e0..bd8f4118ef8 100644 --- a/src/libsystemd/sd-event/sd-event.c +++ b/src/libsystemd/sd-event/sd-event.c @@ -1619,12 +1619,9 @@ _public_ int sd_event_add_child( if (!callback) callback = child_exit_callback; - /* As an optimization we only do these checks on the first child event source created. */ - if (e->n_online_child_sources == 0) { - r = verify_sigchld(options); - if (r < 0) - return r; - } + r = verify_sigchld(options); + if (r < 0) + return r; r = hashmap_ensure_allocated(&e->child_sources, NULL); if (r < 0) @@ -1704,11 +1701,9 @@ _public_ int sd_event_add_child_pidfd( if (!callback) callback = child_exit_callback; - if (e->n_online_child_sources == 0) { - r = verify_sigchld(options); - if (r < 0) - return r; - } + r = verify_sigchld(options); + if (r < 0) + return r; r = hashmap_ensure_allocated(&e->child_sources, NULL); if (r < 0) diff --git a/src/machine/machine.c b/src/machine/machine.c index 6914b912ce9..d18b7e4df82 100644 --- a/src/machine/machine.c +++ b/src/machine/machine.c @@ -1242,14 +1242,13 @@ int machine_copy_from_to_operation( errno_pipe_fd[1] = safe_close(errno_pipe_fd[1]); - // TODO: port to PidRef and donate child rather than destroying it Operation *operation; r = operation_new(manager, machine, &child, errno_pipe_fd[0], &operation); if (r < 0) return r; TAKE_FD(errno_pipe_fd[0]); - pidref_done(&child); + TAKE_PIDREF(child); *ret = operation; return 0; diff --git a/src/machine/operation.c b/src/machine/operation.c index 0e8fd3a5d8b..ae39b99a996 100644 --- a/src/machine/operation.c +++ b/src/machine/operation.c @@ -111,7 +111,7 @@ int operation_new(Manager *manager, Machine *machine, PidRef *child, int errno_f return -ENOMEM; *o = (Operation) { - .pidref = TAKE_PIDREF(*child), + .pidref = *child, .errno_fd = errno_fd, .extra_fd = -EBADF }; diff --git a/src/portable/portable.c b/src/portable/portable.c index c3fdf5d5afb..cc1269de1e3 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -542,7 +542,7 @@ static int portable_extract_by_path( if (r < 0) return r; - TAKE_PIDREF(child); + pidref_done(&child); } if (!os_release) diff --git a/src/portable/portabled-image-bus.c b/src/portable/portabled-image-bus.c index 3dbc094a217..6d361a8b613 100644 --- a/src/portable/portabled-image-bus.c +++ b/src/portable/portabled-image-bus.c @@ -554,7 +554,7 @@ int bus_image_common_remove( if (r < 0) return r; - TAKE_PIDREF(child); + /* We don't need to disarm child cleanup here because operation_new() takes over ownership internally. */ errno_pipe_fd[0] = -EBADF; return 1; diff --git a/src/shared/dissect-image.c b/src/shared/dissect-image.c index 28e40606aee..b6baa0ff784 100644 --- a/src/shared/dissect-image.c +++ b/src/shared/dissect-image.c @@ -4445,7 +4445,7 @@ int dissected_image_acquire_metadata( if (r < 0) goto finish; - TAKE_PIDREF(child); + pidref_done(&child); n = read(error_pipe[0], &v, sizeof(v)); if (n < 0) { diff --git a/src/shared/exec-util.c b/src/shared/exec-util.c index 98c3f6748d4..9221350fdc3 100644 --- a/src/shared/exec-util.c +++ b/src/shared/exec-util.c @@ -174,7 +174,7 @@ static int do_execute( _cleanup_(pidref_freep) PidRef *dup = NULL; r = pidref_dup(&pidref, &dup); if (r < 0) - return r; + return log_error_errno(r, "Failed to duplicate pid reference: %m"); r = hashmap_ensure_put(&pids, &pidref_hash_ops_free_free, dup, t); if (r < 0) @@ -222,11 +222,10 @@ static int do_execute( } while (!hashmap_isempty(pids)) { + _cleanup_(pidref_freep) PidRef *pidref = NULL; _cleanup_free_ char *t = NULL; - void *p; - t = ASSERT_PTR(hashmap_steal_first_key_and_value(pids, &p)); - _cleanup_(pidref_freep) PidRef *pidref = p; + t = ASSERT_PTR(hashmap_steal_first_key_and_value(pids, (void**) &pidref)); r = pidref_wait_for_terminate_and_check(t, pidref, WAIT_LOG); if (r < 0) @@ -275,7 +274,7 @@ int execute_strv( const char *process_name = strjoina("(", name, ")"); - PidRef executor_pidref = PIDREF_NULL; + _cleanup_(pidref_done) PidRef executor_pidref = PIDREF_NULL; r = pidref_safe_fork(process_name, FORK_RESET_SIGNALS|FORK_DEATHSIG_SIGTERM|FORK_LOG, &executor_pidref); if (r < 0) return r; diff --git a/src/shutdown/umount.c b/src/shutdown/umount.c index 9f061177ee3..7729a1d4159 100644 --- a/src/shutdown/umount.c +++ b/src/shutdown/umount.c @@ -294,18 +294,17 @@ static int remount_with_timeout(MountPoint *m, bool last_try) { else if (si.si_code != CLD_EXITED || si.si_status != 0) { /* Try to read error code from child */ r = read_errno(pfd[0]); - if (r == -EIO) + if (r < 0 && r != -EIO) + log_debug_errno(r, + "Remounting '%s' failed abnormally, child process " PID_FMT " failed: %m", + m->path, pidref.pid); + else 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 */ + pidref_done(&pidref); /* child exited (just not as we expected) hence don't kill anymore */ } return r; @@ -364,18 +363,17 @@ static int umount_with_timeout(MountPoint *m, bool last_try) { else if (si.si_code != CLD_EXITED || si.si_status != 0) { /* Try to read error code from child */ r = read_errno(pfd[0]); - if (r == -EIO) + if (r < 0 && r != -EIO) + log_debug_errno(r, + "Unmounting '%s' failed abnormally, child process " PID_FMT " failed: %m", + m->path, pidref.pid); + else 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 */ + pidref_done(&pidref); /* It died, but abnormally, no purpose in killing */ } return r; diff --git a/src/vmspawn/vmspawn.c b/src/vmspawn/vmspawn.c index 69ea071a819..291cac3be15 100644 --- a/src/vmspawn/vmspawn.c +++ b/src/vmspawn/vmspawn.c @@ -1085,7 +1085,7 @@ static int on_child_exit(sd_event_source *s, const siginfo_t *si, void *userdata si->si_pid, signal_to_string(si->si_status)); else ret = log_error_errno(SYNTHETIC_ERRNO(EPROTO), - "Got unexpected exit code %i from child,", + "Got unexpected exit code %i from child.", si->si_code); /* Regardless of whether the main qemu process or an auxiliary process died, let's exit either way