From 51894d706f88ca8138a427ac83b7384b24e01ac0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 27 Sep 2017 19:56:36 +0200 Subject: [PATCH 1/6] service: fix main_pid_good() comment We don't actually return -1, don't claim that. --- src/core/service.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 1a4455bd22..b6f929051e 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1401,8 +1401,7 @@ static int service_spawn( static int main_pid_good(Service *s) { assert(s); - /* Returns 0 if the pid is dead, 1 if it is good, -1 if we - * don't know */ + /* Returns 0 if the pid is dead, > 0 if it is good, < 0 if we don't know */ /* If we know the pid file, then let's just check if it is * still valid */ From 07697d7ec501ccca25281b30c26b4b2dcbc357e7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 2 Oct 2017 12:57:03 +0200 Subject: [PATCH 2/6] service: add explanatory comments to control_pid_good() and cgroup_good() Let's add a similar comment to each as we already have for main_pid_good(), emphasizing that these functions are supposed to be have very similar. --- src/core/service.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/core/service.c b/src/core/service.c index b6f929051e..56e07de69e 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1425,6 +1425,10 @@ static int main_pid_good(Service *s) { _pure_ static int control_pid_good(Service *s) { assert(s); + /* Returns 0 if the control PID is dead, > 0 if it is good. We never actually return < 0 here, but in order to + * make this function as similar as possible to main_pid_good() and cgroup_good(), we pretend that < 0 also + * means: we can't figure it out. */ + return s->control_pid > 0; } @@ -1433,6 +1437,9 @@ static int cgroup_good(Service *s) { assert(s); + /* Returns 0 if the cgroup is empty or doesn't exist, > 0 if it is exists and is populated, < 0 if we can't + * figure it out */ + if (!UNIT(s)->cgroup_path) return 0; From 3c751b1bfaf734db09256a5631f1f9ce75cf0d35 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 27 Sep 2017 19:56:57 +0200 Subject: [PATCH 3/6] service: a cgroup empty notification isn't reason enough to go down The processes associated with a service are not just the ones in its cgroup, but also the control and main processes, which might possibly live outside of it, for example if they transitioned into their own cgroups because they registered a PAM session of their own. Hence, if we get a cgroup empty notification always check if the main PID is still around before taking action too eagerly. Fixes: #6045 --- src/core/service.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 56e07de69e..82d9bb585b 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2871,7 +2871,9 @@ static void service_notify_cgroup_empty_event(Unit *u) { * SIGCHLD for. */ case SERVICE_START: - if (s->type == SERVICE_NOTIFY) { + if (s->type == SERVICE_NOTIFY && + main_pid_good(s) == 0 && + control_pid_good(s) == 0) { /* No chance of getting a ready notification anymore */ service_enter_stop_post(s, SERVICE_FAILURE_PROTOCOL); break; @@ -2880,7 +2882,10 @@ static void service_notify_cgroup_empty_event(Unit *u) { /* Fall through */ case SERVICE_START_POST: - if (s->pid_file_pathspec) { + if (s->pid_file_pathspec && + main_pid_good(s) == 0 && + control_pid_good(s) == 0) { + /* Give up hoping for the daemon to write its PID file */ log_unit_warning(u, "Daemon never wrote its PID file. Failing."); From 019be286767fae799eac20d77cc3982e0622729e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 29 Sep 2017 16:39:46 +0200 Subject: [PATCH 4/6] service: drop _pure_ decorator on static function The compiler should be good enough to figure this out on its own if this is a static function, and it makes control_pid_good() an outlier anyway, and decorators like this tend to bitrot. Hence, to keep things simple and automatic, let's just drop the decorator. --- src/core/service.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/service.c b/src/core/service.c index 82d9bb585b..7880c6b446 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1422,7 +1422,7 @@ static int main_pid_good(Service *s) { return -EAGAIN; } -_pure_ static int control_pid_good(Service *s) { +static int control_pid_good(Service *s) { assert(s); /* Returns 0 if the control PID is dead, > 0 if it is good. We never actually return < 0 here, but in order to From b13ddbbcf35197651c9a60fba58d4d89e18ee769 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 29 Sep 2017 16:41:34 +0200 Subject: [PATCH 5/6] service: accept the fact that the three xyz_good() functions return ints Currently, all three of cgroup_good(), main_pid_good(), control_pid_good() all return an "int" (two of them propagate errors). It's a good thing to keep the three functions similar, so let's leave it at that, but then let's clean up the invocation of the three functions so that they always clearly acknowledge that the return value is not a bool, but potentially negative. --- src/core/service.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 7880c6b446..ffbfc5c459 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1447,7 +1447,7 @@ static int cgroup_good(Service *s) { if (r < 0) return r; - return !r; + return r == 0; } static bool service_shall_restart(Service *s) { @@ -2906,7 +2906,7 @@ static void service_notify_cgroup_empty_event(Unit *u) { case SERVICE_STOP_SIGTERM: case SERVICE_STOP_SIGKILL: - if (main_pid_good(s) <= 0 && !control_pid_good(s)) + if (main_pid_good(s) <= 0 && control_pid_good(s) <= 0) service_enter_stop_post(s, SERVICE_SUCCESS); break; @@ -2914,7 +2914,7 @@ static void service_notify_cgroup_empty_event(Unit *u) { case SERVICE_STOP_POST: case SERVICE_FINAL_SIGTERM: case SERVICE_FINAL_SIGKILL: - if (main_pid_good(s) <= 0 && !control_pid_good(s)) + if (main_pid_good(s) <= 0 && control_pid_good(s) <= 0) service_enter_dead(s, SERVICE_SUCCESS, true); break; @@ -3051,7 +3051,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { case SERVICE_STOP_SIGTERM: case SERVICE_STOP_SIGKILL: - if (!control_pid_good(s)) + if (control_pid_good(s) <= 0) service_enter_stop_post(s, f); /* If there is still a control process, wait for that first */ @@ -3061,7 +3061,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { case SERVICE_FINAL_SIGTERM: case SERVICE_FINAL_SIGKILL: - if (!control_pid_good(s)) + if (control_pid_good(s) <= 0) service_enter_dead(s, f, true); break; @@ -3142,7 +3142,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { r = service_load_pid_file(s, !has_start_post); if (!has_start_post && r < 0) { r = service_demand_pid_file(s); - if (r < 0 || !cgroup_good(s)) + if (r < 0 || cgroup_good(s) == 0) service_enter_signal(s, SERVICE_STOP_SIGTERM, SERVICE_FAILURE_PROTOCOL); break; } @@ -3164,7 +3164,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { r = service_load_pid_file(s, true); if (r < 0) { r = service_demand_pid_file(s); - if (r < 0 || !cgroup_good(s)) + if (r < 0 || cgroup_good(s) == 0) service_enter_stop(s, SERVICE_FAILURE_PROTOCOL); break; } From 5aaeeffb5f17dd025b79be573bd0e613fdf340d9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 29 Sep 2017 16:56:51 +0200 Subject: [PATCH 6/6] man: document that PAMName= and NotifyAccess=all don't mix well. See: #6045 --- man/systemd.exec.xml | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml index f0b48e4a41..06ae6b3252 100644 --- a/man/systemd.exec.xml +++ b/man/systemd.exec.xml @@ -936,7 +936,18 @@ Note that for each unit making use of this option a PAM session handler process will be maintained as part of the unit and stays around as long as the unit is active, to ensure that appropriate actions can be taken when the unit and hence the PAM session terminates. This process is named (sd-pam) and - is an immediate child process of the unit's main process. + is an immediate child process of the unit's main process. + + Note that when this option is used for a unit it is very likely (depending on PAM configuration) that the + main unit process will be migrated to its own session scope unit when it is activated. This process will hence + be associated with two units: the unit it was originally started from (and for which + PAMName= was configured), and the session scope unit. Any child processes of that process + will however be associated with the session scope unit only. This has implications when used in combination + with NotifyAccess=, as these child processes will not be able to affect + changes in the original unit through notification messages. These messages will be considered belonging to the + session scope unit and not the original unit. It is hence not recommended to use PAMName= in + combination with NotifyAccess=. +