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
This commit is contained in:
Daan De Meyer
2025-12-21 08:16:03 +01:00
parent 471fe52d57
commit f9708c53aa
10 changed files with 31 additions and 40 deletions
+3 -3
View File
@@ -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) {
+6 -11
View File
@@ -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)
+1 -2
View File
@@ -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;
+1 -1
View File
@@ -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
};
+1 -1
View File
@@ -542,7 +542,7 @@ static int portable_extract_by_path(
if (r < 0)
return r;
TAKE_PIDREF(child);
pidref_done(&child);
}
if (!os_release)
+1 -1
View File
@@ -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;
+1 -1
View File
@@ -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) {
+4 -5
View File
@@ -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;
+12 -14
View File
@@ -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;
+1 -1
View File
@@ -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