core: when forcibly killing/aborting left-over unit processes log about it

Let's lot at LOG_NOTICE about any processes that we are going to
SIGKILL/SIGABRT because clean termination of them didn't work.

This turns the various boolean flag parameters to cg_kill(), cg_migrate() and
related calls into a single binary flags parameter, simply because the function
now gained even more parameters and the parameter listed shouldn't get too
long.

Logging for killing processes is done either when the kill signal is SIGABRT or
SIGKILL, or on explicit request if KILL_TERMINATE_AND_LOG instead of LOG_TERMINATE
is passed. This isn't used yet in this patch, but is made use of in a later
patch.

diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c
index 7cdc97e..630e15b 100644 (file)
--- a/src/basic/cgroup-util.c
+++ b/src/basic/cgroup-util.c
@@ -197,7 +197,15 @@ int cg_rmdir(const char *controller, const char *path) {
         return 0;
 }
 
-int cg_kill(const char *controller, const char *path, int sig, bool sigcont, bool ignore_self, Set *s) {
+int cg_kill(
+                const char *controller,
+                const char *path,
+                int sig,
+                CGroupFlags flags,
+                Set *s,
+                cg_kill_log_func_t log_kill,
+                void *userdata) {
+
         _cleanup_set_free_ Set *allocated_set = NULL;
         bool done = false;
         int r, ret = 0;
@@ -232,19 +240,22 @@ int cg_kill(const char *controller, const char *path, int sig, bool sigcont, boo
 
                 while ((r = cg_read_pid(f, &pid)) > 0) {
 
-                        if (ignore_self && pid == my_pid)
+                        if ((flags & CGROUP_IGNORE_SELF) && pid == my_pid)
                                 continue;
 
                         if (set_get(s, PID_TO_PTR(pid)) == PID_TO_PTR(pid))
                                 continue;
 
+                        if (log_kill)
+                                log_kill(pid, sig, userdata);
+
                         /* If we haven't killed this process yet, kill
                          * it */
                         if (kill(pid, sig) < 0) {
                                 if (ret >= 0 && errno != ESRCH)
                                         ret = -errno;
                         } else {
-                                if (sigcont && sig != SIGKILL)
+                                if (flags & CGROUP_SIGCONT)
                                         (void) kill(pid, SIGCONT);
 
                                 if (ret == 0)
@@ -278,7 +289,15 @@ int cg_kill(const char *controller, const char *path, int sig, bool sigcont, boo
         return ret;
 }
 
-int cg_kill_recursive(const char *controller, const char *path, int sig, bool sigcont, bool ignore_self, bool rem, Set *s) {
+int cg_kill_recursive(
+                const char *controller,
+                const char *path,
+                int sig,
+                CGroupFlags flags,
+                Set *s,
+                cg_kill_log_func_t log_kill,
+                void *userdata) {
+
         _cleanup_set_free_ Set *allocated_set = NULL;
         _cleanup_closedir_ DIR *d = NULL;
         int r, ret;
@@ -293,7 +312,7 @@ int cg_kill_recursive(const char *controller, const char *path, int sig, bool si
                         return -ENOMEM;
         }
 
-        ret = cg_kill(controller, path, sig, sigcont, ignore_self, s);
+        ret = cg_kill(controller, path, sig, flags, s, log_kill, userdata);
 
         r = cg_enumerate_subgroups(controller, path, &d);
         if (r < 0) {
@@ -311,15 +330,14 @@ int cg_kill_recursive(const char *controller, const char *path, int sig, bool si
                 if (!p)
                         return -ENOMEM;
 
-                r = cg_kill_recursive(controller, p, sig, sigcont, ignore_self, rem, s);
+                r = cg_kill_recursive(controller, p, sig, flags, s, log_kill, userdata);
                 if (r != 0 && ret >= 0)
                         ret = r;
         }
-
         if (ret >= 0 && r < 0)
                 ret = r;
 
-        if (rem) {
+        if (flags & CGROUP_REMOVE) {
                 r = cg_rmdir(controller, path);
                 if (r < 0 && ret >= 0 && r != -ENOENT && r != -EBUSY)
                         return r;
@@ -328,7 +346,13 @@ int cg_kill_recursive(const char *controller, const char *path, int sig, bool si
         return ret;
 }
 
-int cg_migrate(const char *cfrom, const char *pfrom, const char *cto, const char *pto, bool ignore_self) {
+int cg_migrate(
+                const char *cfrom,
+                const char *pfrom,
+                const char *cto,
+                const char *pto,
+                CGroupFlags flags) {
+
         bool done = false;
         _cleanup_set_free_ Set *s = NULL;
         int r, ret = 0;
@@ -363,7 +387,7 @@ int cg_migrate(const char *cfrom, const char *pfrom, const char *cto, const char
                         /* This might do weird stuff if we aren't a
                          * single-threaded program. However, we
                          * luckily know we are not */
-                        if (ignore_self && pid == my_pid)
+                        if ((flags & CGROUP_IGNORE_SELF) && pid == my_pid)
                                 continue;
 
                         if (set_get(s, PID_TO_PTR(pid)) == PID_TO_PTR(pid))
@@ -411,8 +435,7 @@ int cg_migrate_recursive(
                 const char *pfrom,
                 const char *cto,
                 const char *pto,
-                bool ignore_self,
-                bool rem) {
+                CGroupFlags flags) {
 
         _cleanup_closedir_ DIR *d = NULL;
         int r, ret = 0;
@@ -423,7 +446,7 @@ int cg_migrate_recursive(
         assert(cto);
         assert(pto);
 
-        ret = cg_migrate(cfrom, pfrom, cto, pto, ignore_self);
+        ret = cg_migrate(cfrom, pfrom, cto, pto, flags);
 
         r = cg_enumerate_subgroups(cfrom, pfrom, &d);
         if (r < 0) {
@@ -441,7 +464,7 @@ int cg_migrate_recursive(
                 if (!p)
                         return -ENOMEM;
 
-                r = cg_migrate_recursive(cfrom, p, cto, pto, ignore_self, rem);
+                r = cg_migrate_recursive(cfrom, p, cto, pto, flags);
                 if (r != 0 && ret >= 0)
                         ret = r;
         }
@@ -449,7 +472,7 @@ int cg_migrate_recursive(
         if (r < 0 && ret >= 0)
                 ret = r;
 
-        if (rem) {
+        if (flags & CGROUP_REMOVE) {
                 r = cg_rmdir(cfrom, pfrom);
                 if (r < 0 && ret >= 0 && r != -ENOENT && r != -EBUSY)
                         return r;
@@ -463,8 +486,7 @@ int cg_migrate_recursive_fallback(
                 const char *pfrom,
                 const char *cto,
                 const char *pto,
-                bool ignore_self,
-                bool rem) {
+                CGroupFlags flags) {
 
         int r;
 
@@ -473,7 +495,7 @@ int cg_migrate_recursive_fallback(
         assert(cto);
         assert(pto);
 
-        r = cg_migrate_recursive(cfrom, pfrom, cto, pto, ignore_self, rem);
+        r = cg_migrate_recursive(cfrom, pfrom, cto, pto, flags);
         if (r < 0) {
                 char prefix[strlen(pto) + 1];
 
@@ -482,7 +504,7 @@ int cg_migrate_recursive_fallback(
                 PATH_FOREACH_PREFIX(prefix, pto) {
                         int q;
 
-                        q = cg_migrate_recursive(cfrom, pfrom, cto, prefix, ignore_self, rem);
+                        q = cg_migrate_recursive(cfrom, pfrom, cto, prefix, flags);
                         if (q >= 0)
                                 return q;
                 }
@@ -1955,7 +1977,7 @@ int cg_migrate_everywhere(CGroupMask supported, const char *from, const char *to
         int r = 0, unified;
 
         if (!path_equal(from, to))  {
-                r = cg_migrate_recursive(SYSTEMD_CGROUP_CONTROLLER, from, SYSTEMD_CGROUP_CONTROLLER, to, false, true);
+                r = cg_migrate_recursive(SYSTEMD_CGROUP_CONTROLLER, from, SYSTEMD_CGROUP_CONTROLLER, to, CGROUP_REMOVE);
                 if (r < 0)
                         return r;
         }
@@ -1979,7 +2001,7 @@ int cg_migrate_everywhere(CGroupMask supported, const char *from, const char *to
                 if (!p)
                         p = to;
 
-                (void) cg_migrate_recursive_fallback(SYSTEMD_CGROUP_CONTROLLER, to, cgroup_controller_to_string(c), p, false, false);
+                (void) cg_migrate_recursive_fallback(SYSTEMD_CGROUP_CONTROLLER, to, cgroup_controller_to_string(c), p, 0);
         }
 
         return 0;

diff --git a/src/basic/cgroup-util.h b/src/basic/cgroup-util.h
index 4bb5291..14ebde5 100644 (file)
--- a/src/basic/cgroup-util.h
+++ b/src/basic/cgroup-util.h
@@ -135,12 +135,20 @@ int cg_read_event(const char *controller, const char *path, const char *event,
 int cg_enumerate_subgroups(const char *controller, const char *path, DIR **_d);
 int cg_read_subgroup(DIR *d, char **fn);
 
-int cg_kill(const char *controller, const char *path, int sig, bool sigcont, bool ignore_self, Set *s);
-int cg_kill_recursive(const char *controller, const char *path, int sig, bool sigcont, bool ignore_self, bool remove, Set *s);
+typedef enum CGroupFlags {
+        CGROUP_SIGCONT     = 1,
+        CGROUP_IGNORE_SELF = 2,
+        CGROUP_REMOVE      = 4,
+} CGroupFlags;
 
-int cg_migrate(const char *cfrom, const char *pfrom, const char *cto, const char *pto, bool ignore_self);
-int cg_migrate_recursive(const char *cfrom, const char *pfrom, const char *cto, const char *pto, bool ignore_self, bool remove);
-int cg_migrate_recursive_fallback(const char *cfrom, const char *pfrom, const char *cto, const char *pto, bool ignore_self, bool rem);
+typedef void (*cg_kill_log_func_t)(pid_t pid, int sig, void *userdata);
+
+int cg_kill(const char *controller, const char *path, int sig, CGroupFlags flags, Set *s, cg_kill_log_func_t kill_log, void *userdata);
+int cg_kill_recursive(const char *controller, const char *path, int sig, CGroupFlags flags, Set *s, cg_kill_log_func_t kill_log, void *userdata);
+
+int cg_migrate(const char *cfrom, const char *pfrom, const char *cto, const char *pto, CGroupFlags flags);
+int cg_migrate_recursive(const char *cfrom, const char *pfrom, const char *cto, const char *pto, CGroupFlags flags);
+int cg_migrate_recursive_fallback(const char *cfrom, const char *pfrom, const char *cto, const char *pto, CGroupFlags flags);
 
 int cg_split_spec(const char *spec, char **controller, char **path);
 int cg_mangle_path(const char *path, char **result);

diff --git a/src/core/cgroup.c b/src/core/cgroup.c
index 94d1161..8b0f11e 100644 (file)
--- a/src/core/cgroup.c
+++ b/src/core/cgroup.c
@@ -1705,7 +1705,7 @@ int manager_setup_cgroup(Manager *m) {
 
                 /* also, move all other userspace processes remaining
                  * in the root cgroup into that scope. */
-                r = cg_migrate(SYSTEMD_CGROUP_CONTROLLER, m->cgroup_root, SYSTEMD_CGROUP_CONTROLLER, scope_path, false);
+                r = cg_migrate(SYSTEMD_CGROUP_CONTROLLER, m->cgroup_root, SYSTEMD_CGROUP_CONTROLLER, scope_path, 0);
                 if (r < 0)
                         log_warning_errno(r, "Couldn't move remaining userspace processes, ignoring: %m");
 

diff --git a/src/core/scope.c b/src/core/scope.c
index decd1a1..66a5058 100644 (file)
--- a/src/core/scope.c
+++ b/src/core/scope.c
@@ -240,7 +240,7 @@ static void scope_enter_signal(Scope *s, ScopeState state, ScopeResult f) {
 
         /* If we have a controller set let's ask the controller nicely
          * to terminate the scope, instead of us going directly into
-         * SIGTERM beserk mode */
+         * SIGTERM berserk mode */
         if (state == SCOPE_STOP_SIGTERM)
                 skip_signal = bus_scope_send_request_stop(s) > 0;
 

diff --git a/src/core/service.c b/src/core/service.c
index 13de671..afb1985 100644 (file)
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -1674,7 +1674,7 @@ static void service_kill_control_processes(Service *s) {
                 return;
 
         p = strjoina(UNIT(s)->cgroup_path, "/control");
-        cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, p, SIGKILL, true, true, true, NULL);
+        cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, p, SIGKILL, CGROUP_SIGCONT|CGROUP_IGNORE_SELF|CGROUP_REMOVE, NULL, NULL, NULL);
 }
 
 static void service_enter_start(Service *s) {

diff --git a/src/core/unit.c b/src/core/unit.c
index fdf7ce3..4934a0e 100644 (file)
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -3144,7 +3144,7 @@ int unit_kill_common(
                 if (!pid_set)
                         return -ENOMEM;
 
-                q = cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, signo, false, false, false, pid_set);
+                q = cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, signo, 0, pid_set, NULL, NULL);
                 if (q < 0 && q != -EAGAIN && q != -ESRCH && q != -ENOENT)
                         r = q;
                 else
@@ -3512,6 +3512,43 @@ int unit_make_transient(Unit *u) {
         return 0;
 }
 
+static void log_kill(pid_t pid, int sig, void *userdata) {
+        _cleanup_free_ char *comm = NULL;
+
+        (void) get_process_comm(pid, &comm);
+
+        /* Don't log about processes marked with brackets, under the assumption that these are temporary processes
+           only, like for example systemd's own PAM stub process. */
+        if (comm && comm[0] == '(')
+                return;
+
+        log_unit_notice(userdata,
+                        "Killing process " PID_FMT " (%s) with signal SIG%s.",
+                        pid,
+                        strna(comm),
+                        signal_to_string(sig));
+}
+
+static int operation_to_signal(KillContext *c, KillOperation k) {
+        assert(c);
+
+        switch (k) {
+
+        case KILL_TERMINATE:
+        case KILL_TERMINATE_AND_LOG:
+                return c->kill_signal;
+
+        case KILL_KILL:
+                return SIGKILL;
+
+        case KILL_ABORT:
+                return SIGABRT;
+
+        default:
+                assert_not_reached("KillOperation unknown");
+        }
+}
+
 int unit_kill_context(
                 Unit *u,
                 KillContext *c,
@@ -3520,58 +3557,63 @@ int unit_kill_context(
                 pid_t control_pid,
                 bool main_pid_alien) {
 
-        bool wait_for_exit = false;
+        bool wait_for_exit = false, send_sighup;
+        cg_kill_log_func_t log_func;
         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. */
+
         if (c->kill_mode == KILL_NONE)
                 return 0;
 
-        switch (k) {
-        case KILL_KILL:
-                sig = SIGKILL;
-                break;
-        case KILL_ABORT:
-                sig = SIGABRT;
-                break;
-        case KILL_TERMINATE:
-                sig = c->kill_signal;
-                break;
-        default:
-                assert_not_reached("KillOperation unknown");
-        }
+        sig = operation_to_signal(c, k);
+
+        send_sighup =
+                c->send_sighup &&
+                IN_SET(k, KILL_TERMINATE, KILL_TERMINATE_AND_LOG) &&
+                sig != SIGHUP;
+
+        log_func =
+                k != KILL_TERMINATE ||
+                IN_SET(sig, SIGKILL, SIGABRT) ? log_kill : NULL;
 
         if (main_pid > 0) {
-                r = kill_and_sigcont(main_pid, sig);
+                if (log_func)
+                        log_func(main_pid, sig, u);
 
+                r = kill_and_sigcont(main_pid, sig);
                 if (r < 0 && r != -ESRCH) {
                         _cleanup_free_ char *comm = NULL;
-                        get_process_comm(main_pid, &comm);
+                        (void) get_process_comm(main_pid, &comm);
 
                         log_unit_warning_errno(u, r, "Failed to kill main process " PID_FMT " (%s), ignoring: %m", main_pid, strna(comm));
                 } else {
                         if (!main_pid_alien)
                                 wait_for_exit = true;
 
-                        if (c->send_sighup && k == KILL_TERMINATE)
+                        if (r != -ESRCH && send_sighup)
                                 (void) kill(main_pid, SIGHUP);
                 }
         }
 
         if (control_pid > 0) {
-                r = kill_and_sigcont(control_pid, sig);
+                if (log_func)
+                        log_func(control_pid, sig, u);
 
+                r = kill_and_sigcont(control_pid, sig);
                 if (r < 0 && r != -ESRCH) {
                         _cleanup_free_ char *comm = NULL;
-                        get_process_comm(control_pid, &comm);
+                        (void) get_process_comm(control_pid, &comm);
 
                         log_unit_warning_errno(u, r, "Failed to kill control process " PID_FMT " (%s), ignoring: %m", control_pid, strna(comm));
                 } else {
                         wait_for_exit = true;
 
-                        if (c->send_sighup && k == KILL_TERMINATE)
+                        if (r != -ESRCH && send_sighup)
                                 (void) kill(control_pid, SIGHUP);
                 }
         }
@@ -3585,7 +3627,11 @@ int unit_kill_context(
                 if (!pid_set)
                         return -ENOMEM;
 
-                r = cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, sig, true, k != KILL_TERMINATE, false, pid_set);
+                r = cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path,
+                                      sig,
+                                      CGROUP_SIGCONT|CGROUP_IGNORE_SELF,
+                                      pid_set,
+                                      log_func, u);
                 if (r < 0) {
                         if (r != -EAGAIN && r != -ESRCH && r != -ENOENT)
                                 log_unit_warning_errno(u, r, "Failed to kill control group %s, ignoring: %m", u->cgroup_path);
@@ -3610,14 +3656,18 @@ int unit_kill_context(
                              (detect_container() == 0 && !unit_cgroup_delegate(u)))
                                 wait_for_exit = true;
 
-                        if (c->send_sighup && k != KILL_KILL) {
+                        if (send_sighup) {
                                 set_free(pid_set);
 
                                 pid_set = unit_pid_set(main_pid, control_pid);
                                 if (!pid_set)
                                         return -ENOMEM;
 
-                                cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, SIGHUP, false, true, false, pid_set);
+                                cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path,
+                                                  SIGHUP,
+                                                  CGROUP_IGNORE_SELF,
+                                                  pid_set,
+                                                  NULL, NULL);
                         }
                 }
         }

diff --git a/src/core/unit.h b/src/core/unit.h
index c41011e..1eabfa5 100644 (file)
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -36,6 +36,7 @@ typedef struct UnitStatusMessageFormats UnitStatusMessageFormats;
 
 typedef enum KillOperation {
         KILL_TERMINATE,
+        KILL_TERMINATE_AND_LOG,
         KILL_KILL,
         KILL_ABORT,
         _KILL_OPERATION_MAX,

diff --git a/src/test/test-cgroup.c b/src/test/test-cgroup.c
index 72c32d9..5336c19 100644 (file)
--- a/src/test/test-cgroup.c
+++ b/src/test/test-cgroup.c
@@ -60,16 +60,16 @@ int main(int argc, char*argv[]) {
         assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a") > 0);
         assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b") == 0);
 
-        assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a", 0, false, false, false, NULL) == 0);
-        assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", 0, false, false, false, NULL) > 0);
+        assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a", 0, 0, NULL, NULL, NULL) == 0);
+        assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", 0, 0, NULL, NULL, NULL) > 0);
 
-        assert_se(cg_migrate_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", SYSTEMD_CGROUP_CONTROLLER, "/test-a", false, false) > 0);
+        assert_se(cg_migrate_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", SYSTEMD_CGROUP_CONTROLLER, "/test-a", 0) > 0);
 
         assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a") == 0);
         assert_se(cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b") > 0);
 
-        assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a", 0, false, false, false, NULL) > 0);
-        assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", 0, false, false, false, NULL) == 0);
+        assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-a", 0, 0, NULL, NULL, NULL) > 0);
+        assert_se(cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, "/test-b", 0, 0, NULL, NULL, NULL) == 0);
 
         cg_trim(SYSTEMD_CGROUP_CONTROLLER, "/", false);
 

diff --git a/src/udev/udevd.c b/src/udev/udevd.c
index a8ab208..a893a2b 100644 (file)
--- a/src/udev/udevd.c
+++ b/src/udev/udevd.c
@@ -1256,7 +1256,7 @@ static int on_post(sd_event_source *s, void *userdata) {
                                         return r;
                         } else if (manager->cgroup)
                                 /* cleanup possible left-over processes in our cgroup */
-                                cg_kill(SYSTEMD_CGROUP_CONTROLLER, manager->cgroup, SIGKILL, false, true, NULL);
+                                cg_kill(SYSTEMD_CGROUP_CONTROLLER, manager->cgroup, SIGKILL, CGROUP_IGNORE_SELF, NULL, NULL, NULL);
                 }
         }