From 37eb258e91c780fd6fe5e44110abd9da71dce6de Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sun, 10 Sep 2023 16:25:02 +0200 Subject: [PATCH] core: port unit_main_pid() + unit_control_pid() to PidRef and drop unit_kill_common() This ports over unit_main_pid() + unit_control_pid() to return PidRef* pointers (which also means the underlying UnitVTable function pointers are changed accordingly). This then uses te functions to simplify the unit_kill() call, by avoiding the kill() vtable indirection and instead just suing unit_main_pid() and unit_control_pid() directly. --- TODO | 2 -- src/core/dbus-service.c | 9 +++-- src/core/dbus-unit.c | 11 +++--- src/core/mount.c | 17 ++------- src/core/scope.c | 6 ---- src/core/service.c | 25 +++---------- src/core/slice.c | 6 ---- src/core/socket.c | 13 ++----- src/core/swap.c | 13 ++----- src/core/unit.c | 78 +++++++++++++++++++---------------------- src/core/unit.h | 13 +++---- 11 files changed, 60 insertions(+), 133 deletions(-) diff --git a/TODO b/TODO index c75d65e0e46..bb126377982 100644 --- a/TODO +++ b/TODO @@ -174,9 +174,7 @@ Features: - pid_is_unwaited() → pidref_is_unwaited() - pid_is_alive() → pidref_is_alive() - unit_watch_pid() → unit_watch_pidref() - - unit_kill_common() - actually wait for POLLIN on piref's pidfd in service logic - - unit_main_pid() + unit_control_pid() - exec_spawn() - serialization of control/main pid in service, socket, mount, swap units - unit_fork_and_watch_rm_rf() diff --git a/src/core/dbus-service.c b/src/core/dbus-service.c index cc65a559903..5bc487bc399 100644 --- a/src/core/dbus-service.c +++ b/src/core/dbus-service.c @@ -134,7 +134,6 @@ static int bus_service_method_mount(sd_bus_message *message, void *userdata, sd_ int read_only, make_file_or_directory; Unit *u = ASSERT_PTR(userdata); ExecContext *c; - pid_t unit_pid; int r; assert(message); @@ -192,14 +191,14 @@ static int bus_service_method_mount(sd_bus_message *message, void *userdata, sd_ if (!exec_needs_mount_namespace(c, NULL, unit_get_exec_runtime(u))) return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "Unit not running in private mount namespace, cannot activate bind mount"); - unit_pid = unit_main_pid(u); - if (unit_pid == 0 || !UNIT_IS_ACTIVE_OR_RELOADING(unit_active_state(u))) + PidRef* unit_pid = unit_main_pid(u); + if (!pidref_is_set(unit_pid) || !UNIT_IS_ACTIVE_OR_RELOADING(unit_active_state(u))) return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "Unit is not running"); propagate_directory = strjoina("/run/systemd/propagate/", u->id); if (is_image) r = mount_image_in_namespace( - unit_pid, + unit_pid->pid, propagate_directory, "/run/systemd/incoming/", src, dest, @@ -209,7 +208,7 @@ static int bus_service_method_mount(sd_bus_message *message, void *userdata, sd_ c->mount_image_policy ?: &image_policy_service); else r = bind_mount_in_namespace( - unit_pid, + unit_pid->pid, propagate_directory, "/run/systemd/incoming/", src, dest, diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 1f673fe26d2..e9b446945aa 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -1331,7 +1331,6 @@ int bus_unit_method_get_processes(sd_bus_message *message, void *userdata, sd_bu _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; _cleanup_set_free_ Set *pids = NULL; Unit *u = userdata; - pid_t pid; int r; assert(message); @@ -1359,16 +1358,16 @@ int bus_unit_method_get_processes(sd_bus_message *message, void *userdata, sd_bu } /* The main and control pids might live outside of the cgroup, hence fetch them separately */ - pid = unit_main_pid(u); - if (pid > 0) { - r = append_process(reply, NULL, pid, pids); + PidRef *pid = unit_main_pid(u); + if (pidref_is_set(pid)) { + r = append_process(reply, NULL, pid->pid, pids); if (r < 0) return r; } pid = unit_control_pid(u); - if (pid > 0) { - r = append_process(reply, NULL, pid, pids); + if (pidref_is_set(pid)) { + r = append_process(reply, NULL, pid->pid, pids); if (r < 0) return r; } diff --git a/src/core/mount.c b/src/core/mount.c index c156a84c1d3..4af2e6810e1 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -2213,20 +2213,8 @@ static void mount_reset_failed(Unit *u) { m->clean_result = MOUNT_SUCCESS; } -static int mount_kill(Unit *u, KillWho who, int signo, int code, int value, sd_bus_error *error) { - Mount *m = MOUNT(u); - - assert(m); - - return unit_kill_common(u, who, signo, code, value, -1, m->control_pid.pid, error); -} - -static int mount_control_pid(Unit *u) { - Mount *m = MOUNT(u); - - assert(m); - - return m->control_pid.pid; +static PidRef* mount_control_pid(Unit *u) { + return &ASSERT_PTR(MOUNT(u))->control_pid; } static int mount_clean(Unit *u, ExecCleanMask mask) { @@ -2358,7 +2346,6 @@ const UnitVTable mount_vtable = { .stop = mount_stop, .reload = mount_reload, - .kill = mount_kill, .clean = mount_clean, .can_clean = mount_can_clean, diff --git a/src/core/scope.c b/src/core/scope.c index 31ea5c54832..13907855c50 100644 --- a/src/core/scope.c +++ b/src/core/scope.c @@ -533,10 +533,6 @@ static void scope_reset_failed(Unit *u) { s->result = SCOPE_SUCCESS; } -static int scope_kill(Unit *u, KillWho who, int signo, int code, int value, sd_bus_error *error) { - return unit_kill_common(u, who, signo, code, value, -1, -1, error); -} - static int scope_get_timeout(Unit *u, usec_t *timeout) { Scope *s = SCOPE(u); usec_t t; @@ -830,8 +826,6 @@ const UnitVTable scope_vtable = { .start = scope_start, .stop = scope_stop, - .kill = scope_kill, - .freeze = unit_freeze_vtable_common, .thaw = unit_thaw_vtable_common, diff --git a/src/core/service.c b/src/core/service.c index 34948fa9234..0ecfe46cfdf 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -4825,28 +4825,12 @@ static void service_reset_failed(Unit *u) { s->flush_n_restarts = false; } -static int service_kill(Unit *u, KillWho who, int signo, int code, int value, sd_bus_error *error) { - Service *s = SERVICE(u); - - assert(s); - - return unit_kill_common(u, who, signo, code, value, s->main_pid.pid, s->control_pid.pid, error); +static PidRef* service_main_pid(Unit *u) { + return &ASSERT_PTR(SERVICE(u))->main_pid; } -static int service_main_pid(Unit *u) { - Service *s = SERVICE(u); - - assert(s); - - return s->main_pid.pid; -} - -static int service_control_pid(Unit *u) { - Service *s = SERVICE(u); - - assert(s); - - return s->control_pid.pid; +static PidRef* service_control_pid(Unit *u) { + return &ASSERT_PTR(SERVICE(u))->control_pid; } static bool service_needs_console(Unit *u) { @@ -5163,7 +5147,6 @@ const UnitVTable service_vtable = { .can_reload = service_can_reload, - .kill = service_kill, .clean = service_clean, .can_clean = service_can_clean, diff --git a/src/core/slice.c b/src/core/slice.c index c6d142cebba..c7701b4017f 100644 --- a/src/core/slice.c +++ b/src/core/slice.c @@ -247,10 +247,6 @@ static int slice_stop(Unit *u) { return 1; } -static int slice_kill(Unit *u, KillWho who, int signo, int code, int value, sd_bus_error *error) { - return unit_kill_common(u, who, signo, code, value, -1, -1, error); -} - static int slice_serialize(Unit *u, FILE *f, FDSet *fds) { Slice *s = SLICE(u); @@ -436,8 +432,6 @@ const UnitVTable slice_vtable = { .start = slice_start, .stop = slice_stop, - .kill = slice_kill, - .freeze = slice_freeze, .thaw = slice_thaw, .can_freeze = slice_can_freeze, diff --git a/src/core/socket.c b/src/core/socket.c index d885581247c..861e580b3fe 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -3382,10 +3382,6 @@ static void socket_trigger_notify(Unit *u, Unit *other) { socket_set_state(s, SOCKET_RUNNING); } -static int socket_kill(Unit *u, KillWho who, int signo, int code, int value, sd_bus_error *error) { - return unit_kill_common(u, who, signo, code, value, -1, SOCKET(u)->control_pid.pid, error); -} - static int socket_get_timeout(Unit *u, usec_t *timeout) { Socket *s = SOCKET(u); usec_t t; @@ -3414,12 +3410,8 @@ char *socket_fdname(Socket *s) { return s->fdname ?: UNIT(s)->id; } -static int socket_control_pid(Unit *u) { - Socket *s = SOCKET(u); - - assert(s); - - return s->control_pid.pid; +static PidRef *socket_control_pid(Unit *u) { + return &ASSERT_PTR(SOCKET(u))->control_pid; } static int socket_clean(Unit *u, ExecCleanMask mask) { @@ -3576,7 +3568,6 @@ const UnitVTable socket_vtable = { .start = socket_start, .stop = socket_stop, - .kill = socket_kill, .clean = socket_clean, .can_clean = socket_can_clean, diff --git a/src/core/swap.c b/src/core/swap.c index d8c08375761..e06de629da3 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -1476,10 +1476,6 @@ static void swap_reset_failed(Unit *u) { s->clean_result = SWAP_SUCCESS; } -static int swap_kill(Unit *u, KillWho who, int signo, int code, int value, sd_bus_error *error) { - return unit_kill_common(u, who, signo, code, value, -1, SWAP(u)->control_pid.pid, error); -} - static int swap_get_timeout(Unit *u, usec_t *timeout) { Swap *s = SWAP(u); usec_t t; @@ -1516,12 +1512,8 @@ static bool swap_supported(void) { return supported; } -static int swap_control_pid(Unit *u) { - Swap *s = SWAP(u); - - assert(s); - - return s->control_pid.pid; +static PidRef* swap_control_pid(Unit *u) { + return &ASSERT_PTR(SWAP(u))->control_pid; } static int swap_clean(Unit *u, ExecCleanMask mask) { @@ -1638,7 +1630,6 @@ const UnitVTable swap_vtable = { .start = swap_start, .stop = swap_stop, - .kill = swap_kill, .clean = swap_clean, .can_clean = swap_can_clean, diff --git a/src/core/unit.c b/src/core/unit.c index c542e4f5042..c17e0990184 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2943,7 +2943,7 @@ void unit_unwatch_all_pids(Unit *u) { } static void unit_tidy_watch_pids(Unit *u) { - pid_t except1, except2; + PidRef *except1, *except2; void *e; assert(u); @@ -2956,7 +2956,8 @@ static void unit_tidy_watch_pids(Unit *u) { SET_FOREACH(e, u->pids) { pid_t pid = PTR_TO_PID(e); - if (pid == except1 || pid == except2) + if ((pidref_is_set(except1) && pid == except1->pid) || + (pidref_is_set(except2) && pid == except2->pid)) continue; if (!pid_is_unwaited(pid)) @@ -3958,18 +3959,6 @@ bool unit_will_restart(Unit *u) { return UNIT_VTABLE(u)->will_restart(u); } -int unit_kill(Unit *u, KillWho w, int signo, int code, int value, sd_bus_error *error) { - assert(u); - assert(w >= 0 && w < _KILL_WHO_MAX); - assert(SIGNAL_VALID(signo)); - assert(IN_SET(code, SI_USER, SI_QUEUE)); - - if (!UNIT_VTABLE(u)->kill) - return -EOPNOTSUPP; - - return UNIT_VTABLE(u)->kill(u, w, signo, code, value, error); -} - void unit_notify_cgroup_oom(Unit *u, bool managed_oom) { assert(u); @@ -4012,35 +4001,34 @@ static int kill_common_log(pid_t pid, int signo, void *userdata) { return 1; } -static int kill_or_sigqueue(pid_t pid, int signo, int code, int value) { - assert(pid > 0); +static int kill_or_sigqueue(PidRef* pidref, int signo, int code, int value) { + assert(pidref_is_set(pidref)); assert(SIGNAL_VALID(signo)); switch (code) { case SI_USER: - log_debug("Killing " PID_FMT " with signal SIG%s.", pid, signal_to_string(signo)); - return RET_NERRNO(kill(pid, signo)); + log_debug("Killing " PID_FMT " with signal SIG%s.", pidref->pid, signal_to_string(signo)); + return pidref_kill(pidref, signo); case SI_QUEUE: - log_debug("Enqueuing value %i to " PID_FMT " on signal SIG%s.", value, pid, signal_to_string(signo)); - return RET_NERRNO(sigqueue(pid, signo, (const union sigval) { .sival_int = value })); + log_debug("Enqueuing value %i to " PID_FMT " on signal SIG%s.", value, pidref->pid, signal_to_string(signo)); + return pidref_sigqueue(pidref, signo, value); default: assert_not_reached(); } } -int unit_kill_common( +int unit_kill( Unit *u, KillWho who, int signo, int code, int value, - pid_t main_pid, - pid_t control_pid, sd_bus_error *error) { + PidRef *main_pid, *control_pid; bool killed = false; int ret = 0, r; @@ -4054,24 +4042,30 @@ int unit_kill_common( assert(SIGNAL_VALID(signo)); assert(IN_SET(code, SI_USER, SI_QUEUE)); + main_pid = unit_main_pid(u); + control_pid = unit_control_pid(u); + + if (!UNIT_HAS_CGROUP_CONTEXT(u) && !main_pid && !control_pid) + return sd_bus_error_setf(error, SD_BUS_ERROR_NOT_SUPPORTED, "Unit type does not support process killing."); + if (IN_SET(who, KILL_MAIN, KILL_MAIN_FAIL)) { - if (main_pid < 0) + if (!main_pid) return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_PROCESS, "%s units have no main processes", unit_type_to_string(u->type)); - if (main_pid == 0) + if (!pidref_is_set(main_pid)) return sd_bus_error_set_const(error, BUS_ERROR_NO_SUCH_PROCESS, "No main process to kill"); } if (IN_SET(who, KILL_CONTROL, KILL_CONTROL_FAIL)) { - if (control_pid < 0) + if (!control_pid) return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_PROCESS, "%s units have no control processes", unit_type_to_string(u->type)); - if (control_pid == 0) + if (!pidref_is_set(control_pid)) return sd_bus_error_set_const(error, BUS_ERROR_NO_SUCH_PROCESS, "No control process to kill"); } - if (control_pid > 0 && + if (pidref_is_set(control_pid) && IN_SET(who, KILL_CONTROL, KILL_CONTROL_FAIL, KILL_ALL, KILL_ALL_FAIL)) { _cleanup_free_ char *comm = NULL; - (void) get_process_comm(control_pid, &comm); + (void) get_process_comm(control_pid->pid, &comm); r = kill_or_sigqueue(control_pid, signo, code, value); if (r < 0) { @@ -4081,23 +4075,23 @@ int unit_kill_common( sd_bus_error_set_errnof( error, r, "Failed to send signal SIG%s to control process " PID_FMT " (%s): %m", - signal_to_string(signo), control_pid, strna(comm)); + signal_to_string(signo), control_pid->pid, strna(comm)); log_unit_warning_errno( u, r, "Failed to send signal SIG%s to control process " PID_FMT " (%s) on client request: %m", - signal_to_string(signo), control_pid, strna(comm)); + signal_to_string(signo), control_pid->pid, strna(comm)); } else { log_unit_info(u, "Sent signal SIG%s to control process " PID_FMT " (%s) on client request.", - signal_to_string(signo), control_pid, strna(comm)); + signal_to_string(signo), control_pid->pid, strna(comm)); killed = true; } } - if (main_pid > 0 && + if (pidref_is_set(main_pid) && IN_SET(who, KILL_MAIN, KILL_MAIN_FAIL, KILL_ALL, KILL_ALL_FAIL)) { _cleanup_free_ char *comm = NULL; - (void) get_process_comm(main_pid, &comm); + (void) get_process_comm(main_pid->pid, &comm); r = kill_or_sigqueue(main_pid, signo, code, value); if (r < 0) { @@ -4107,17 +4101,17 @@ int unit_kill_common( sd_bus_error_set_errnof( error, r, "Failed to send signal SIG%s to main process " PID_FMT " (%s): %m", - signal_to_string(signo), main_pid, strna(comm)); + signal_to_string(signo), main_pid->pid, strna(comm)); } log_unit_warning_errno( u, r, "Failed to send signal SIG%s to main process " PID_FMT " (%s) on client request: %m", - signal_to_string(signo), main_pid, strna(comm)); + signal_to_string(signo), main_pid->pid, strna(comm)); } else { log_unit_info(u, "Sent signal SIG%s to main process " PID_FMT " (%s) on client request.", - signal_to_string(signo), main_pid, strna(comm)); + signal_to_string(signo), main_pid->pid, strna(comm)); killed = true; } } @@ -4129,7 +4123,7 @@ int unit_kill_common( _cleanup_set_free_ Set *pid_set = NULL; /* Exclude the main/control pids from being killed via the cgroup */ - pid_set = unit_pid_set(main_pid, control_pid); + pid_set = unit_pid_set(main_pid ? main_pid->pid : 0, control_pid ? control_pid->pid : 0); if (!pid_set) return log_oom(); @@ -5120,22 +5114,22 @@ bool unit_is_pristine(Unit *u) { !u->merged_into; } -pid_t unit_control_pid(Unit *u) { +PidRef* unit_control_pid(Unit *u) { assert(u); if (UNIT_VTABLE(u)->control_pid) return UNIT_VTABLE(u)->control_pid(u); - return 0; + return NULL; } -pid_t unit_main_pid(Unit *u) { +PidRef* unit_main_pid(Unit *u) { assert(u); if (UNIT_VTABLE(u)->main_pid) return UNIT_VTABLE(u)->main_pid(u); - return 0; + return NULL; } static void unit_unref_uid_internal( diff --git a/src/core/unit.h b/src/core/unit.h index 7ce61eb5fd1..d0d713f6126 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -625,8 +625,6 @@ typedef struct UnitVTable { int (*stop)(Unit *u); int (*reload)(Unit *u); - int (*kill)(Unit *u, KillWho w, int signo, int code, int value, sd_bus_error *error); - /* Clear out the various runtime/state/cache/logs/configuration data */ int (*clean)(Unit *u, ExecCleanMask m); @@ -720,10 +718,10 @@ typedef struct UnitVTable { usec_t (*get_timeout_start_usec)(Unit *u); /* Returns the main PID if there is any defined, or 0. */ - pid_t (*main_pid)(Unit *u); + PidRef* (*main_pid)(Unit *u); - /* Returns the main PID if there is any defined, or 0. */ - pid_t (*control_pid)(Unit *u); + /* Returns the control PID if there is any defined, or 0. */ + PidRef* (*control_pid)(Unit *u); /* Returns true if the unit currently needs access to the console */ bool (*needs_console)(Unit *u); @@ -914,7 +912,6 @@ int unit_stop(Unit *u); int unit_reload(Unit *u); int unit_kill(Unit *u, KillWho w, int signo, int code, int value, sd_bus_error *error); -int unit_kill_common(Unit *u, KillWho who, int signo, int code, int value, pid_t main_pid, pid_t control_pid, sd_bus_error *error); void unit_notify_cgroup_oom(Unit *u, bool managed_oom); @@ -1007,8 +1004,8 @@ bool unit_is_unneeded(Unit *u); bool unit_is_upheld_by_active(Unit *u, Unit **ret_culprit); bool unit_is_bound_by_inactive(Unit *u, Unit **ret_culprit); -pid_t unit_control_pid(Unit *u); -pid_t unit_main_pid(Unit *u); +PidRef* unit_control_pid(Unit *u); +PidRef* unit_main_pid(Unit *u); void unit_warn_if_dir_nonempty(Unit *u, const char* where); int unit_fail_if_noncanonical(Unit *u, const char* where);