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 8ed455545114c96657c8a8e79155af113cf43e31..10ed181fcf53aa210da41f963705fba97bc46bbc 100644 (file)
--- a/src/home/homework-luks.c
+++ b/src/home/homework-luks.c
@@ -7,7 +7,6 @@
 #include <sys/ioctl.h>
 #include <sys/xattr.h>
 #include <unistd.h>
-#include "pidref.h"
 #if HAVE_VALGRIND_MEMCHECK_H
 #include <valgrind/memcheck.h>
 #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 8c549eb22e0057873c62ef8acb1b853cef739683..bd8f4118ef860fe401a12e444dd1a8ef5a8db9a9 100644 (file)
--- 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 6914b912ce913f442cf0b9d792f0ee2a3c62ae60..d18b7e4df823a3cb3df4a7c6557d347693603880 100644 (file)
--- 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 0e8fd3a5d8b8ba90dcbef9a155ac92e0448ee52f..ae39b99a996bd1ac70e17b1f3898737d9bb07691 100644 (file)
--- 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 c3fdf5d5afb428f32f8be67dbab281ae7b45e94f..cc1269de1e3c9c3064d65b600dd6c5e371d07356 100644 (file)
--- 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 3dbc094a217467e0ff45d36985df0a27c3bc0f28..6d361a8b6131aa1dbdee9168ab961ee4a8a8ae80 100644 (file)
--- 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 28e40606aee85d7f5e1bbf473799918feabd6ae0..b6baa0ff7841489635819fe2a061aad77f1afa5a 100644 (file)
--- 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 98c3f6748d4f2912181638e597082d0beafe9a5d..9221350fdc336edbdb0d1f2223b01f8aaa2a422c 100644 (file)
--- 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 9f061177ee3e3999b89e69094601d179fa24247f..7729a1d4159064c1be7358f122ee8f7094bb330f 100644 (file)
--- 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 69ea071a8190f123580f8ccf1c15c8db16ed6602..291cac3be15256ba81fbc9572bed1e311427b39d 100644 (file)
--- 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