From fe80d62657339b3f84b42fb20139da5550eff944 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 24 Jan 2024 15:27:52 +0900 Subject: [PATCH 1/3] core/unit: split out unit_kill_context_one() No functional change, just refactoring. --- src/core/unit.c | 78 ++++++++++++++++++++++++++----------------------- 1 file changed, 42 insertions(+), 36 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 6496fc96d41..9334e8a127c 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4746,6 +4746,44 @@ static int operation_to_signal( } } +static int unit_kill_context_one( + Unit *u, + const PidRef *pidref, + const char *type, + bool is_alien, + int sig, + bool send_sighup, + cg_kill_log_func_t log_func) { + + int r; + + assert(u); + assert(type); + + /* This returns > 0 if it makes sense to wait for SIGCHLD for the process, == 0 if not. */ + + if (!pidref_is_set(pidref)) + return 0; + + if (log_func) + log_func(pidref, sig, u); + + r = pidref_kill_and_sigcont(pidref, sig); + if (r == -ESRCH) + return !is_alien; + if (r < 0) { + _cleanup_free_ char *comm = NULL; + + (void) pidref_get_comm(pidref, &comm); + return log_unit_warning_errno(u, r, "Failed to kill %s process " PID_FMT " (%s), ignoring: %m", type, pidref->pid, strna(comm)); + } + + if (send_sighup) + (void) pidref_kill(pidref, SIGHUP); + + return !is_alien; +} + int unit_kill_context(Unit *u, KillOperation k) { bool wait_for_exit = false, send_sighup; cg_kill_log_func_t log_func = NULL; @@ -4773,43 +4811,11 @@ int unit_kill_context(Unit *u, KillOperation k) { 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); + r = unit_kill_context_one(u, main_pid, "main", is_alien, sig, send_sighup, log_func); + wait_for_exit = wait_for_exit || r > 0; - r = pidref_kill_and_sigcont(main_pid, sig); - if (r < 0 && r != -ESRCH) { - _cleanup_free_ char *comm = NULL; - (void) pidref_get_comm(main_pid, &comm); - - log_unit_warning_errno(u, r, "Failed to kill main process " PID_FMT " (%s), ignoring: %m", main_pid->pid, strna(comm)); - } else { - if (!is_alien) - wait_for_exit = true; - - if (r != -ESRCH && send_sighup) - (void) pidref_kill(main_pid, SIGHUP); - } - } - - PidRef *control_pid = unit_control_pid(u); - if (pidref_is_set(control_pid)) { - if (log_func) - log_func(control_pid, sig, u); - - r = pidref_kill_and_sigcont(control_pid, sig); - if (r < 0 && r != -ESRCH) { - _cleanup_free_ char *comm = NULL; - (void) pidref_get_comm(control_pid, &comm); - - log_unit_warning_errno(u, r, "Failed to kill control process " PID_FMT " (%s), ignoring: %m", control_pid->pid, strna(comm)); - } else { - wait_for_exit = true; - - if (r != -ESRCH && send_sighup) - (void) pidref_kill(control_pid, SIGHUP); - } - } + r = unit_kill_context_one(u, unit_control_pid(u), "control", /* is_alien = */ false, sig, send_sighup, log_func); + wait_for_exit = wait_for_exit || r > 0; if (u->cgroup_path && (c->kill_mode == KILL_CONTROL_GROUP || (c->kill_mode == KILL_MIXED && k == KILL_KILL))) { From ec5e2a139372cc539e4944221a0f9919271fea0d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 25 Jan 2024 16:41:37 +0900 Subject: [PATCH 2/3] core/unit: split out unit_kill_one() This also renames 'error' -> 'ret_error'. No functional change, just refactoring. --- src/core/unit.c | 147 ++++++++++++++++++++++++------------------------ src/core/unit.h | 2 +- 2 files changed, 74 insertions(+), 75 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 9334e8a127c..68b57845e01 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4000,13 +4000,55 @@ static int kill_or_sigqueue(PidRef* pidref, int signo, int code, int value) { } } +static int unit_kill_one( + Unit *u, + PidRef *pidref, + const char *type, + int signo, + int code, + int value, + sd_bus_error *ret_error) { + + int r; + + assert(u); + assert(type); + + if (!pidref_is_set(pidref)) + return 0; + + _cleanup_free_ char *comm = NULL; + (void) pidref_get_comm(pidref, &comm); + + r = kill_or_sigqueue(pidref, signo, code, value); + if (r == -ESRCH) + return 0; + if (r < 0) { + /* Report this failure both to the logs and to the client */ + if (ret_error) + sd_bus_error_set_errnof( + ret_error, r, + "Failed to send signal SIG%s to %s process " PID_FMT " (%s): %m", + signal_to_string(signo), type, pidref->pid, strna(comm)); + + return log_unit_warning_errno( + u, r, + "Failed to send signal SIG%s to %s process " PID_FMT " (%s) on client request: %m", + signal_to_string(signo), type, pidref->pid, strna(comm)); + } + + log_unit_info(u, "Sent signal SIG%s to %s process " PID_FMT " (%s) on client request.", + signal_to_string(signo), type, pidref->pid, strna(comm)); + return 1; /* killed */ +} + int unit_kill( Unit *u, KillWho who, int signo, int code, int value, - sd_bus_error *error) { + sd_bus_error *ret_error) { PidRef *main_pid, *control_pid; bool killed = false; @@ -4026,73 +4068,32 @@ int unit_kill( 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."); + return sd_bus_error_setf(ret_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) - return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_PROCESS, "%s units have no main processes", unit_type_to_string(u->type)); + return sd_bus_error_setf(ret_error, BUS_ERROR_NO_SUCH_PROCESS, "%s units have no main processes", unit_type_to_string(u->type)); if (!pidref_is_set(main_pid)) - return sd_bus_error_set_const(error, BUS_ERROR_NO_SUCH_PROCESS, "No main process to kill"); + return sd_bus_error_set_const(ret_error, BUS_ERROR_NO_SUCH_PROCESS, "No main process to kill"); } if (IN_SET(who, KILL_CONTROL, KILL_CONTROL_FAIL)) { 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)); + return sd_bus_error_setf(ret_error, BUS_ERROR_NO_SUCH_PROCESS, "%s units have no control processes", unit_type_to_string(u->type)); if (!pidref_is_set(control_pid)) - return sd_bus_error_set_const(error, BUS_ERROR_NO_SUCH_PROCESS, "No control process to kill"); + return sd_bus_error_set_const(ret_error, BUS_ERROR_NO_SUCH_PROCESS, "No control process to kill"); } - 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) pidref_get_comm(control_pid, &comm); - - r = kill_or_sigqueue(control_pid, signo, code, value); - if (r < 0) { - ret = r; - - /* Report this failure both to the logs and to the client */ - 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->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->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->pid, strna(comm)); - killed = true; - } + if (IN_SET(who, KILL_CONTROL, KILL_CONTROL_FAIL, KILL_ALL, KILL_ALL_FAIL)) { + r = unit_kill_one(u, control_pid, "control", signo, code, value, ret_error); + RET_GATHER(ret, r); + killed = killed || r > 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) pidref_get_comm(main_pid, &comm); - - r = kill_or_sigqueue(main_pid, signo, code, value); - if (r < 0) { - if (ret == 0) { - ret = r; - - 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->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->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->pid, strna(comm)); - killed = true; - } + if (IN_SET(who, KILL_MAIN, KILL_MAIN_FAIL, KILL_ALL, KILL_ALL_FAIL)) { + r = unit_kill_one(u, main_pid, "main", signo, code, value, ret >= 0 ? ret_error : NULL); + RET_GATHER(ret, r); + killed = killed || r > 0; } /* Note: if we shall enqueue rather than kill we won't do this via the cgroup mechanism, since it @@ -4107,29 +4108,27 @@ int unit_kill( return log_oom(); r = cg_kill_recursive(u->cgroup_path, signo, 0, pid_set, kill_common_log, u); - if (r < 0) { - if (!IN_SET(r, -ESRCH, -ENOENT)) { - if (ret == 0) { - ret = r; - - sd_bus_error_set_errnof( - error, r, - "Failed to send signal SIG%s to auxiliary processes: %m", - signal_to_string(signo)); - } - - log_unit_warning_errno( - u, r, - "Failed to send signal SIG%s to auxiliary processes on client request: %m", + if (r < 0 && !IN_SET(r, -ESRCH, -ENOENT)) { + if (ret >= 0) + sd_bus_error_set_errnof( + ret_error, r, + "Failed to send signal SIG%s to auxiliary processes: %m", signal_to_string(signo)); - } - } else - killed = true; + + log_unit_warning_errno( + u, r, + "Failed to send signal SIG%s to auxiliary processes on client request: %m", + signal_to_string(signo)); + + RET_GATHER(ret, r); + } + + killed = killed || r >= 0; } /* If the "fail" versions of the operation are requested, then complain if the set of processes we killed is empty */ - if (ret == 0 && !killed && IN_SET(who, KILL_ALL_FAIL, KILL_CONTROL_FAIL, KILL_MAIN_FAIL)) - return sd_bus_error_set_const(error, BUS_ERROR_NO_SUCH_PROCESS, "No matching processes to kill"); + if (ret >= 0 && !killed && IN_SET(who, KILL_ALL_FAIL, KILL_CONTROL_FAIL, KILL_MAIN_FAIL)) + return sd_bus_error_set_const(ret_error, BUS_ERROR_NO_SUCH_PROCESS, "No matching processes to kill"); return ret; } diff --git a/src/core/unit.h b/src/core/unit.h index 1d43acc0183..f90a8d0b7f6 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -925,7 +925,7 @@ int unit_start(Unit *u, ActivationDetails *details); 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(Unit *u, KillWho w, int signo, int code, int value, sd_bus_error *ret_error); void unit_notify_cgroup_oom(Unit *u, bool managed_oom); From ea1e0bf10bbfb89388a3001afa0b4c28a64bb24e Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 24 Jan 2024 16:11:28 +0900 Subject: [PATCH 3/3] core: introduce unit_unwatch_pidref_done() helper function No functional change, just refactoring. --- src/core/mount.c | 7 +------ src/core/service.c | 14 ++------------ src/core/socket.c | 7 +------ src/core/swap.c | 7 +------ src/core/unit.c | 10 ++++++++++ src/core/unit.h | 1 + 6 files changed, 16 insertions(+), 30 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index d840414d1a3..6c3f4be981a 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -218,12 +218,7 @@ static int mount_arm_timer(Mount *m, bool relative, usec_t usec) { static void mount_unwatch_control_pid(Mount *m) { assert(m); - - if (!pidref_is_set(&m->control_pid)) - return; - - unit_unwatch_pidref(UNIT(m), &m->control_pid); - pidref_done(&m->control_pid); + unit_unwatch_pidref_done(UNIT(m), &m->control_pid); } static void mount_parameters_done(MountParameters *p) { diff --git a/src/core/service.c b/src/core/service.c index 5ebaddf1882..5ac866032c0 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -152,22 +152,12 @@ static void service_init(Unit *u) { static void service_unwatch_control_pid(Service *s) { assert(s); - - if (!pidref_is_set(&s->control_pid)) - return; - - unit_unwatch_pidref(UNIT(s), &s->control_pid); - pidref_done(&s->control_pid); + unit_unwatch_pidref_done(UNIT(s), &s->control_pid); } static void service_unwatch_main_pid(Service *s) { assert(s); - - if (!pidref_is_set(&s->main_pid)) - return; - - unit_unwatch_pidref(UNIT(s), &s->main_pid); - pidref_done(&s->main_pid); + unit_unwatch_pidref_done(UNIT(s), &s->main_pid); } static void service_unwatch_pid_file(Service *s) { diff --git a/src/core/socket.c b/src/core/socket.c index 57f6ab18db2..85bd757b7d0 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -108,12 +108,7 @@ static void socket_init(Unit *u) { static void socket_unwatch_control_pid(Socket *s) { assert(s); - - if (!pidref_is_set(&s->control_pid)) - return; - - unit_unwatch_pidref(UNIT(s), &s->control_pid); - pidref_done(&s->control_pid); + unit_unwatch_pidref_done(UNIT(s), &s->control_pid); } static void socket_cleanup_fd_list(SocketPort *p) { diff --git a/src/core/swap.c b/src/core/swap.c index 0022a3d0ac7..33f6cb2536a 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -152,12 +152,7 @@ static void swap_init(Unit *u) { static void swap_unwatch_control_pid(Swap *s) { assert(s); - - if (!pidref_is_set(&s->control_pid)) - return; - - unit_unwatch_pidref(UNIT(s), &s->control_pid); - pidref_done(&s->control_pid); + unit_unwatch_pidref_done(UNIT(s), &s->control_pid); } static void swap_done(Unit *u) { diff --git a/src/core/unit.c b/src/core/unit.c index 68b57845e01..12b36ebbfd8 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2934,6 +2934,16 @@ void unit_unwatch_all_pids(Unit *u) { u->pids = set_free(u->pids); } +void unit_unwatch_pidref_done(Unit *u, PidRef *pidref) { + assert(u); + + if (!pidref_is_set(pidref)) + return; + + unit_unwatch_pidref(u, pidref); + pidref_done(pidref); +} + static void unit_tidy_watch_pids(Unit *u) { PidRef *except1, *except2, *e; diff --git a/src/core/unit.h b/src/core/unit.h index f90a8d0b7f6..976d79251a4 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -936,6 +936,7 @@ int unit_watch_pid(Unit *u, pid_t pid, bool exclusive); 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); +void unit_unwatch_pidref_done(Unit *u, PidRef *pidref); int unit_enqueue_rewatch_pids(Unit *u); void unit_dequeue_rewatch_pids(Unit *u);