From 8aff7ac4a7315ad7d4d1cf0c564885ba90d6818e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 16 Oct 2020 17:16:02 +0200 Subject: [PATCH 1/3] core: add comment explaining unit_kill_context() vs. unit_kill_common() a bit --- src/core/unit.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index fd73ad2949f..052c11c00f4 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4331,17 +4331,21 @@ int unit_kill_common( int r = 0; bool killed = false; + /* This is the common implementation for explicit user-requested killing of unit processes, shared by + * various unit types. Do not confuse with unit_kill_context(), which is what we use when we want to + * stop a service ourselves. */ + if (IN_SET(who, KILL_MAIN, KILL_MAIN_FAIL)) { if (main_pid < 0) return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_PROCESS, "%s units have no main processes", unit_type_to_string(u->type)); - else if (main_pid == 0) + if (main_pid == 0) 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) return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_PROCESS, "%s units have no control processes", unit_type_to_string(u->type)); - else if (control_pid == 0) + if (control_pid == 0) return sd_bus_error_set_const(error, BUS_ERROR_NO_SUCH_PROCESS, "No control process to kill"); } @@ -4964,8 +4968,9 @@ int unit_kill_context( 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. */ + /* 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) return 0; From 2ae0508e6dab3bb61893132fa936c1f62af8aa29 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 16 Oct 2020 17:16:23 +0200 Subject: [PATCH 2/3] core: correct handling of "systemctl kill --kill-who=main-fail" --kill-who=main-fail never worked correctly, due to a copy and paste mistake in ac5e3a505e49c80b56c971a8fc13bacac961640d, where the same item was listed twice. The mistake was later noticed, but fixed incorrectly, in 201f0c916d8f65ad2595a651b1371fcd39a4cf55. Let's list all *-fail types correctly, finally. And while we are at it, add a nice comment and generate a prettier D-Bus error about this. --- src/core/unit.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 052c11c00f4..f3d4731a46a 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4381,8 +4381,9 @@ int unit_kill_common( killed = true; } - if (r == 0 && !killed && IN_SET(who, KILL_ALL_FAIL, KILL_CONTROL_FAIL)) - return -ESRCH; + /* If the "fail" versions of the operation are requested, then complain if the set of processes we killed is empty */ + if (r == 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"); return r; } From d991100291b6d5b81176417a42c0ca6a17cf6ab8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 16 Oct 2020 17:20:20 +0200 Subject: [PATCH 3/3] core: log about "systemctl kill" requests let's add informational logging about each client requested signal sending. While we are at, let's beef up error handling/log messages in this case quite a bit: let's log errors both to syslog and report errors back to client. Fixes: #17254 --- src/core/unit.c | 75 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 64 insertions(+), 11 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index f3d4731a46a..c67654bbae4 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4320,6 +4320,19 @@ static Set *unit_pid_set(pid_t main_pid, pid_t control_pid) { return TAKE_PTR(pid_set); } +static int kill_common_log(pid_t pid, int signo, void *userdata) { + _cleanup_free_ char *comm = NULL; + Unit *u = userdata; + + assert(u); + + (void) get_process_comm(pid, &comm); + log_unit_info(u, "Sending signal SIG%s to process " PID_FMT " (%s) on client request.", + signal_to_string(signo), pid, strna(comm)); + + return 1; +} + int unit_kill_common( Unit *u, KillWho who, @@ -4351,18 +4364,47 @@ int unit_kill_common( if (IN_SET(who, KILL_CONTROL, KILL_CONTROL_FAIL, KILL_ALL, KILL_ALL_FAIL)) if (control_pid > 0) { - if (kill(control_pid, signo) < 0) - r = -errno; - else + _cleanup_free_ char *comm = NULL; + (void) get_process_comm(control_pid, &comm); + + if (kill(control_pid, signo) < 0) { + /* Report this failure both to the logs and to the client */ + sd_bus_error_set_errnof( + error, errno, + "Failed to send signal SIG%s to control process " PID_FMT " (%s): %m", + signal_to_string(signo), control_pid, strna(comm)); + r = log_unit_warning_errno( + u, errno, + "Failed to send signal SIG%s to control process " PID_FMT " (%s) on client request: %m", + signal_to_string(signo), control_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)); killed = true; + } } if (IN_SET(who, KILL_MAIN, KILL_MAIN_FAIL, KILL_ALL, KILL_ALL_FAIL)) if (main_pid > 0) { - if (kill(main_pid, signo) < 0) - r = -errno; - else + _cleanup_free_ char *comm = NULL; + (void) get_process_comm(main_pid, &comm); + + if (kill(main_pid, signo) < 0) { + if (r == 0) + sd_bus_error_set_errnof( + error, errno, + "Failed to send signal SIG%s to main process " PID_FMT " (%s): %m", + signal_to_string(signo), main_pid, strna(comm)); + + r = log_unit_warning_errno( + u, errno, + "Failed to send signal SIG%s to main process " PID_FMT " (%s) on client request: %m", + signal_to_string(signo), main_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)); killed = true; + } } if (IN_SET(who, KILL_ALL, KILL_ALL_FAIL) && u->cgroup_path) { @@ -4372,12 +4414,23 @@ int unit_kill_common( /* Exclude the main/control pids from being killed via the cgroup */ pid_set = unit_pid_set(main_pid, control_pid); if (!pid_set) - return -ENOMEM; + return log_oom(); - q = cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, signo, 0, pid_set, NULL, NULL); - if (q < 0 && !IN_SET(q, -EAGAIN, -ESRCH, -ENOENT)) - r = q; - else + q = cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, signo, 0, pid_set, kill_common_log, u); + if (q < 0) { + if (!IN_SET(q, -ESRCH, -ENOENT)) { + if (r == 0) + sd_bus_error_set_errnof( + error, q, + "Failed to send signal SIG%s to auxiliary processes: %m", + signal_to_string(signo)); + + r = log_unit_warning_errno( + u, q, + "Failed to send signal SIG%s to auxiliary processes on client request: %m", + signal_to_string(signo)); + } + } else killed = true; }