From d70dfe1b27c00790059265539586e26377782096 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 24 Jan 2024 13:54:45 +0900 Subject: [PATCH 1/6] core: constify PidRef arguments --- src/core/cgroup.c | 6 +++--- src/core/cgroup.h | 6 +++--- src/core/manager.c | 2 +- src/core/manager.h | 2 +- src/core/unit.c | 6 +++--- src/core/unit.h | 6 +++--- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index f82de9b673b..6c37856faf8 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -4202,7 +4202,7 @@ Unit* manager_get_unit_by_cgroup(Manager *m, const char *cgroup) { } } -Unit *manager_get_unit_by_pidref_cgroup(Manager *m, PidRef *pid) { +Unit *manager_get_unit_by_pidref_cgroup(Manager *m, const PidRef *pid) { _cleanup_free_ char *cgroup = NULL; assert(m); @@ -4213,7 +4213,7 @@ Unit *manager_get_unit_by_pidref_cgroup(Manager *m, PidRef *pid) { return manager_get_unit_by_cgroup(m, cgroup); } -Unit *manager_get_unit_by_pidref_watching(Manager *m, PidRef *pid) { +Unit *manager_get_unit_by_pidref_watching(Manager *m, const PidRef *pid) { Unit *u, **array; assert(m); @@ -4232,7 +4232,7 @@ Unit *manager_get_unit_by_pidref_watching(Manager *m, PidRef *pid) { return NULL; } -Unit *manager_get_unit_by_pidref(Manager *m, PidRef *pid) { +Unit *manager_get_unit_by_pidref(Manager *m, const PidRef *pid) { Unit *u; assert(m); diff --git a/src/core/cgroup.h b/src/core/cgroup.h index 0e90b6bd22c..6cb04f4eb26 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -375,9 +375,9 @@ void manager_shutdown_cgroup(Manager *m, bool delete); unsigned manager_dispatch_cgroup_realize_queue(Manager *m); Unit *manager_get_unit_by_cgroup(Manager *m, const char *cgroup); -Unit *manager_get_unit_by_pidref_cgroup(Manager *m, PidRef *pid); -Unit *manager_get_unit_by_pidref_watching(Manager *m, PidRef *pid); -Unit* manager_get_unit_by_pidref(Manager *m, PidRef *pid); +Unit *manager_get_unit_by_pidref_cgroup(Manager *m, const PidRef *pid); +Unit *manager_get_unit_by_pidref_watching(Manager *m, const PidRef *pid); +Unit* manager_get_unit_by_pidref(Manager *m, const PidRef *pid); Unit* manager_get_unit_by_pid(Manager *m, pid_t pid); uint64_t unit_get_ancestor_memory_min(Unit *u); diff --git a/src/core/manager.c b/src/core/manager.c index 462baab36a6..47244a4a26c 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -2405,7 +2405,7 @@ void manager_clear_jobs(Manager *m) { job_finish_and_invalidate(j, JOB_CANCELED, false, false); } -void manager_unwatch_pidref(Manager *m, PidRef *pid) { +void manager_unwatch_pidref(Manager *m, const PidRef *pid) { assert(m); for (;;) { diff --git a/src/core/manager.h b/src/core/manager.h index ba7c1e28c93..9f1a21b22db 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -550,7 +550,7 @@ int manager_propagate_reload(Manager *m, Unit *unit, JobMode mode, sd_bus_error void manager_clear_jobs(Manager *m); -void manager_unwatch_pidref(Manager *m, PidRef *pid); +void manager_unwatch_pidref(Manager *m, const PidRef *pid); unsigned manager_dispatch_load_queue(Manager *m); diff --git a/src/core/unit.c b/src/core/unit.c index d9e67067425..3a3e5f49202 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2788,7 +2788,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su } } -int unit_watch_pidref(Unit *u, PidRef *pid, bool exclusive) { +int unit_watch_pidref(Unit *u, const PidRef *pid, bool exclusive) { _cleanup_(pidref_freep) PidRef *pid_dup = NULL; int r; @@ -2872,7 +2872,7 @@ int unit_watch_pid(Unit *u, pid_t pid, bool exclusive) { return unit_watch_pidref(u, &pidref, exclusive); } -void unit_unwatch_pidref(Unit *u, PidRef *pid) { +void unit_unwatch_pidref(Unit *u, const PidRef *pid) { assert(u); assert(pidref_is_set(pid)); @@ -5910,7 +5910,7 @@ bool unit_needs_console(Unit *u) { return exec_context_may_touch_console(ec); } -int unit_pid_attachable(Unit *u, PidRef *pid, sd_bus_error *error) { +int unit_pid_attachable(Unit *u, const PidRef *pid, sd_bus_error *error) { int r; assert(u); diff --git a/src/core/unit.h b/src/core/unit.h index fdaa3b8a8ef..115ab56eaa2 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -931,9 +931,9 @@ void unit_notify_cgroup_oom(Unit *u, bool managed_oom); void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_success); -int unit_watch_pidref(Unit *u, PidRef *pid, bool exclusive); +int unit_watch_pidref(Unit *u, const PidRef *pid, bool exclusive); int unit_watch_pid(Unit *u, pid_t pid, bool exclusive); -void unit_unwatch_pidref(Unit *u, PidRef *pid); +void unit_unwatch_pidref(Unit *u, const PidRef *pid); void unit_unwatch_pid(Unit *u, pid_t pid); void unit_unwatch_all_pids(Unit *u); @@ -1055,7 +1055,7 @@ int unit_warn_leftover_processes(Unit *u, cg_kill_log_func_t log_func); bool unit_needs_console(Unit *u); -int unit_pid_attachable(Unit *unit, PidRef *pid, sd_bus_error *error); +int unit_pid_attachable(Unit *unit, const PidRef *pid, sd_bus_error *error); static inline bool unit_has_job_type(Unit *u, JobType type) { return u && u->job && u->job->type == type; From 3e22239da7a05c3c0d45fe1059a1a59dd29bcb5f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 24 Jan 2024 14:08:40 +0900 Subject: [PATCH 2/6] core: introduce unit_main_pid_full() which optionally provides if the PID is alien or not --- src/core/service.c | 9 +++++++-- src/core/unit.c | 6 ++++-- src/core/unit.h | 7 +++++-- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index aacfe0d57e8..4f53a0dc060 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -4786,8 +4786,13 @@ static void service_reset_failed(Unit *u) { s->flush_n_restarts = false; } -static PidRef* service_main_pid(Unit *u) { - return &ASSERT_PTR(SERVICE(u))->main_pid; +static PidRef* service_main_pid(Unit *u, bool *ret_is_alien) { + Service *s = ASSERT_PTR(SERVICE(u)); + + if (ret_is_alien) + *ret_is_alien = s->main_pid_alien; + + return &s->main_pid; } static PidRef* service_control_pid(Unit *u) { diff --git a/src/core/unit.c b/src/core/unit.c index 3a3e5f49202..1c7225bb8f1 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -5112,12 +5112,14 @@ PidRef* unit_control_pid(Unit *u) { return NULL; } -PidRef* unit_main_pid(Unit *u) { +PidRef* unit_main_pid_full(Unit *u, bool *ret_is_alien) { assert(u); if (UNIT_VTABLE(u)->main_pid) - return UNIT_VTABLE(u)->main_pid(u); + return UNIT_VTABLE(u)->main_pid(u, ret_is_alien); + if (ret_is_alien) + *ret_is_alien = false; return NULL; } diff --git a/src/core/unit.h b/src/core/unit.h index 115ab56eaa2..eb94ff29641 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -732,7 +732,7 @@ typedef struct UnitVTable { usec_t (*get_timeout_start_usec)(Unit *u); /* Returns the main PID if there is any defined, or 0. */ - PidRef* (*main_pid)(Unit *u); + PidRef* (*main_pid)(Unit *u, bool *ret_is_alien); /* Returns the control PID if there is any defined, or 0. */ PidRef* (*control_pid)(Unit *u); @@ -1021,7 +1021,10 @@ bool unit_is_upheld_by_active(Unit *u, Unit **ret_culprit); bool unit_is_bound_by_inactive(Unit *u, Unit **ret_culprit); PidRef* unit_control_pid(Unit *u); -PidRef* unit_main_pid(Unit *u); +PidRef* unit_main_pid_full(Unit *u, bool *ret_is_alien); +static inline PidRef* unit_main_pid(Unit *u) { + return unit_main_pid_full(u, NULL); +} void unit_warn_if_dir_nonempty(Unit *u, const char* where); int unit_fail_if_noncanonical(Unit *u, const char* where); From b826e317541cd3fc6c0020a2353cc2fb4d0eff0c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 24 Jan 2024 14:02:46 +0900 Subject: [PATCH 3/6] core: use helper functions like unit_main_pid() in unit_kill_context() No functional changes. Just refactoring. --- src/core/mount.c | 8 +------- src/core/scope.c | 6 +----- src/core/service.c | 8 +------- src/core/socket.c | 8 +------- src/core/swap.c | 8 +------- src/core/unit.c | 18 +++++++----------- src/core/unit.h | 2 +- 7 files changed, 13 insertions(+), 45 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 048802fa1be..d840414d1a3 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1033,13 +1033,7 @@ static void mount_enter_signal(Mount *m, MountState state, MountResult f) { if (m->result == MOUNT_SUCCESS) m->result = f; - r = unit_kill_context( - UNIT(m), - &m->kill_context, - state_to_kill_operation(state), - /* main_pid= */ NULL, - &m->control_pid, - /* main_pid_alien= */ false); + r = unit_kill_context(UNIT(m), state_to_kill_operation(state)); if (r < 0) { log_unit_warning_errno(UNIT(m), r, "Failed to kill processes: %m"); goto fail; diff --git a/src/core/scope.c b/src/core/scope.c index e4c27da91d2..8cbbf25801e 100644 --- a/src/core/scope.c +++ b/src/core/scope.c @@ -317,13 +317,9 @@ static void scope_enter_signal(Scope *s, ScopeState state, ScopeResult f) { else { r = unit_kill_context( UNIT(s), - &s->kill_context, state != SCOPE_STOP_SIGTERM ? KILL_KILL : s->was_abandoned ? KILL_TERMINATE_AND_LOG : - KILL_TERMINATE, - /* main_pid= */ NULL, - /* control_pid= */ NULL, - /* main_pid_alien= */ false); + KILL_TERMINATE); if (r < 0) { log_unit_warning_errno(UNIT(s), r, "Failed to kill processes: %m"); goto fail; diff --git a/src/core/service.c b/src/core/service.c index 4f53a0dc060..97b547084ee 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2121,13 +2121,7 @@ static void service_enter_signal(Service *s, ServiceState state, ServiceResult f (void) unit_enqueue_rewatch_pids(UNIT(s)); kill_operation = state_to_kill_operation(s, state); - r = unit_kill_context( - UNIT(s), - &s->kill_context, - kill_operation, - &s->main_pid, - &s->control_pid, - s->main_pid_alien); + r = unit_kill_context(UNIT(s), kill_operation); if (r < 0) { log_unit_warning_errno(UNIT(s), r, "Failed to kill processes: %m"); goto fail; diff --git a/src/core/socket.c b/src/core/socket.c index e539e2ca0be..57f6ab18db2 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -2070,13 +2070,7 @@ static void socket_enter_signal(Socket *s, SocketState state, SocketResult f) { if (s->result == SOCKET_SUCCESS) s->result = f; - r = unit_kill_context( - UNIT(s), - &s->kill_context, - state_to_kill_operation(s, state), - /* main_pid= */ NULL, - &s->control_pid, - /* main_pid_alien= */ false); + r = unit_kill_context(UNIT(s), state_to_kill_operation(s, state)); if (r < 0) { log_unit_warning_errno(UNIT(s), r, "Failed to kill processes: %m"); goto fail; diff --git a/src/core/swap.c b/src/core/swap.c index 9f7c12b0c3c..0022a3d0ac7 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -734,13 +734,7 @@ static void swap_enter_signal(Swap *s, SwapState state, SwapResult f) { if (s->result == SWAP_SUCCESS) s->result = f; - r = unit_kill_context( - UNIT(s), - &s->kill_context, - state_to_kill_operation(s, state), - /* main_pid= */ NULL, - &s->control_pid, - /* main_pid_alien= */ false); + r = unit_kill_context(UNIT(s), state_to_kill_operation(s, state)); if (r < 0) { log_unit_warning_errno(UNIT(s), r, "Failed to kill processes: %m"); goto fail; diff --git a/src/core/unit.c b/src/core/unit.c index 1c7225bb8f1..1e0a3d86302 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4749,26 +4749,19 @@ static int operation_to_signal( } } -int unit_kill_context( - Unit *u, - KillContext *c, - KillOperation k, - PidRef* main_pid, - PidRef* control_pid, - bool main_pid_alien) { - +int unit_kill_context(Unit *u, KillOperation k) { bool wait_for_exit = false, send_sighup; cg_kill_log_func_t log_func = NULL; int sig, r; assert(u); - assert(c); /* Kill the processes belonging to this unit, in preparation for shutting the unit down. Returns > 0 * if we killed something worth waiting for, 0 otherwise. Do not confuse with unit_kill_common() * which is used for user-requested killing of unit processes. */ - if (c->kill_mode == KILL_NONE) + KillContext *c = unit_get_kill_context(u); + if (!c || c->kill_mode == KILL_NONE) return 0; bool noteworthy; @@ -4781,6 +4774,8 @@ int unit_kill_context( IN_SET(k, KILL_TERMINATE, KILL_TERMINATE_AND_LOG) && sig != SIGHUP; + bool is_alien; + PidRef *main_pid = unit_main_pid_full(u, &is_alien); if (pidref_is_set(main_pid)) { if (log_func) log_func(main_pid, sig, u); @@ -4792,7 +4787,7 @@ int unit_kill_context( log_unit_warning_errno(u, r, "Failed to kill main process " PID_FMT " (%s), ignoring: %m", main_pid->pid, strna(comm)); } else { - if (!main_pid_alien) + if (!is_alien) wait_for_exit = true; if (r != -ESRCH && send_sighup) @@ -4800,6 +4795,7 @@ int unit_kill_context( } } + PidRef *control_pid = unit_control_pid(u); if (pidref_is_set(control_pid)) { if (log_func) log_func(control_pid, sig, u); diff --git a/src/core/unit.h b/src/core/unit.h index eb94ff29641..1d43acc0183 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -1006,7 +1006,7 @@ char* unit_concat_strv(char **l, UnitWriteFlags flags); int unit_write_setting(Unit *u, UnitWriteFlags flags, const char *name, const char *data); int unit_write_settingf(Unit *u, UnitWriteFlags mode, const char *name, const char *format, ...) _printf_(4,5); -int unit_kill_context(Unit *u, KillContext *c, KillOperation k, PidRef *main_pid, PidRef *control_pid, bool main_pid_alien); +int unit_kill_context(Unit *u, KillOperation k); int unit_make_transient(Unit *u); From ae6a9e650c47c2f5a1bddca85ec7a562f847e24d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 24 Jan 2024 14:33:41 +0900 Subject: [PATCH 4/6] unit: modernize unit_pid_set() --- src/core/unit.c | 47 +++++++++++++++++++++-------------------------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 1e0a3d86302..c199318b6eb 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -3948,28 +3948,25 @@ void unit_notify_cgroup_oom(Unit *u, bool managed_oom) { UNIT_VTABLE(u)->notify_cgroup_oom(u, managed_oom); } -static Set *unit_pid_set(pid_t main_pid, pid_t control_pid) { - _cleanup_set_free_ Set *pid_set = NULL; +static int unit_pid_set(Unit *u, Set **pid_set) { int r; - pid_set = set_new(NULL); - if (!pid_set) - return NULL; + assert(u); + assert(pid_set); + + set_clear(*pid_set); /* This updates input. */ /* Exclude the main/control pids from being killed via the cgroup */ - if (main_pid > 0) { - r = set_put(pid_set, PID_TO_PTR(main_pid)); - if (r < 0) - return NULL; - } - if (control_pid > 0) { - r = set_put(pid_set, PID_TO_PTR(control_pid)); - if (r < 0) - return NULL; - } + PidRef *pid; + FOREACH_POINTER(pid, unit_main_pid(u), unit_control_pid(u)) + if (pidref_is_set(pid)) { + r = set_ensure_put(pid_set, NULL, PID_TO_PTR(pid->pid)); + if (r < 0) + return r; + } - return TAKE_PTR(pid_set); + return 0; } static int kill_common_log(const PidRef *pid, int signo, void *userdata) { @@ -4105,8 +4102,8 @@ int unit_kill( _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 ? main_pid->pid : 0, control_pid ? control_pid->pid : 0); - if (!pid_set) + r = unit_pid_set(u, &pid_set); + if (r < 0) return log_oom(); r = cg_kill_recursive(u->cgroup_path, signo, 0, pid_set, kill_common_log, u); @@ -4819,9 +4816,9 @@ int unit_kill_context(Unit *u, KillOperation k) { _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 ? main_pid->pid : 0, control_pid ? control_pid->pid : 0); - if (!pid_set) - return -ENOMEM; + r = unit_pid_set(u, &pid_set); + if (r < 0) + return r; r = cg_kill_recursive( u->cgroup_path, @@ -4847,11 +4844,9 @@ int unit_kill_context(Unit *u, KillOperation k) { wait_for_exit = true; if (send_sighup) { - set_free(pid_set); - - pid_set = unit_pid_set(main_pid ? main_pid->pid : 0, control_pid ? control_pid->pid : 0); - if (!pid_set) - return -ENOMEM; + r = unit_pid_set(u, &pid_set); + if (r < 0) + return r; (void) cg_kill_recursive( u->cgroup_path, From c917a807015ff3e895c1e6865ab04995fb1dbce8 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 24 Jan 2024 14:39:20 +0900 Subject: [PATCH 5/6] core/service: declare 'int r' at the beginning --- src/core/service.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 97b547084ee..5ebaddf1882 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -3701,6 +3701,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { Service *s = SERVICE(u); ServiceResult f; ExitClean clean_mode; + int r; assert(s); assert(pid >= 0); @@ -3952,7 +3953,6 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { if (s->pid_file) { bool has_start_post; - int r; /* Let's try to load the pid file here if we can. * The PID file might actually be created by a START_POST @@ -3979,8 +3979,6 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { } if (s->pid_file) { - int r; - r = service_load_pid_file(s, true); if (r < 0) { r = service_demand_pid_file(s); From 330c080eeb096c8657b0a526cb1e26cd330b030b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 24 Jan 2024 14:40:25 +0900 Subject: [PATCH 6/6] core/exec-invoke: drop unused pam_pid --- src/core/exec-invoke.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index 1ae766d93ed..829e094f519 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -1123,7 +1123,7 @@ static int setup_pam( sigset_t old_ss; int pam_code = PAM_SUCCESS, r; bool close_session = false; - pid_t pam_pid = 0, parent_pid; + pid_t parent_pid; int flags = 0; assert(name); @@ -1198,7 +1198,7 @@ static int setup_pam( parent_pid = getpid_cached(); - r = safe_fork("(sd-pam)", 0, &pam_pid); + r = safe_fork("(sd-pam)", 0, NULL); if (r < 0) goto fail; if (r == 0) {