Merge pull request #7991 from poettering/n-on-console

a comprehensive fix for the n_on_console miscounting issue

5 files changed, 102 insertions, 42 deletions

diff --git a/src/core/manager.c b/src/core/manager.c
index 2952f217c3..4a043fbb11 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -2449,8 +2449,15 @@ static int manager_dispatch_idle_pipe_fd(sd_event_source *source, int fd, uint32
         assert(m);
         assert(m->idle_pipe[2] == fd);
 
+        /* There's at least one Type=idle child that just gave up on us waiting for the boot process to complete. Let's
+         * now turn off any further console output if there's at least one service that needs console access, so that
+         * from now on our own output should not spill into that service's output anymore. After all, we support
+         * Type=idle only to beautify console output and it generally is set on services that want to own the console
+         * exclusively without our interference. */
         m->no_console_output = m->n_on_console > 0;
 
+        /* Acknowledge the child's request, and let all all other children know too that they shouldn't wait any longer
+         * by closing the pipes towards them, which is what they are waiting for. */
         manager_close_idle_pipe(m);
 
         return 0;
@@ -3567,10 +3574,7 @@ static bool manager_get_show_status(Manager *m, StatusType type) {
         if (type != STATUS_TYPE_EMERGENCY && manager_check_ask_password(m) > 0)
                 return false;
 
-        if (m->show_status > 0)
-                return true;
-
-        return false;
+        return m->show_status > 0;
 }
 
 const char *manager_get_confirm_spawn(Manager *m) {
@@ -4094,6 +4098,21 @@ char *manager_taint_string(Manager *m) {
         return buf;
 }
 
+void manager_ref_console(Manager *m) {
+        assert(m);
+
+        m->n_on_console++;
+}
+
+void manager_unref_console(Manager *m) {
+
+        assert(m->n_on_console > 0);
+        m->n_on_console--;
+
+        if (m->n_on_console == 0)
+                m->no_console_output = false; /* unset no_console_output flag, since the console is definitely free now */
+}
+
 static const char *const manager_state_table[_MANAGER_STATE_MAX] = {
         [MANAGER_INITIALIZING] = "initializing",
         [MANAGER_STARTING] = "starting",
diff --git a/src/core/manager.h b/src/core/manager.h
index e8da2286fa..0eed67b46a 100644
--- a/src/core/manager.h
+++ b/src/core/manager.h
@@ -455,6 +455,9 @@ void manager_deserialize_gid_refs_one(Manager *m, const char *value);
 
 char *manager_taint_string(Manager *m);
 
+void manager_ref_console(Manager *m);
+void manager_unref_console(Manager *m);
+
 const char *manager_state_to_string(ManagerState m) _const_;
 ManagerState manager_state_from_string(const char *s) _pure_;
 
diff --git a/src/core/service.c b/src/core/service.c
index 81c776cd15..6476dc68c5 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -1060,26 +1060,6 @@ static void service_set_state(Service *s, ServiceState state) {
         if (state == SERVICE_EXITED && !MANAGER_IS_RELOADING(UNIT(s)->manager))
                 unit_prune_cgroup(UNIT(s));
 
-        /* For remain_after_exit services, let's see if we can "release" the
-         * hold on the console, since unit_notify() only does that in case of
-         * change of state */
-        if (state == SERVICE_EXITED &&
-            s->remain_after_exit &&
-            UNIT(s)->manager->n_on_console > 0) {
-
-                ExecContext *ec;
-
-                ec = unit_get_exec_context(UNIT(s));
-                if (ec && exec_context_may_touch_console(ec)) {
-                        Manager *m = UNIT(s)->manager;
-
-                        m->n_on_console--;
-                        if (m->n_on_console == 0)
-                                /* unset no_console_output flag, since the console is free */
-                                m->no_console_output = false;
-                }
-        }
-
         if (old_state != state)
                 log_unit_debug(UNIT(s), "Changed %s -> %s", service_state_to_string(old_state), service_state_to_string(state));
 
@@ -3806,6 +3786,32 @@ static int service_control_pid(Unit *u) {
         return s->control_pid;
 }
 
+static bool service_needs_console(Unit *u) {
+        Service *s = SERVICE(u);
+
+        assert(s);
+
+        /* We provide our own implementation of this here, instead of relying of the generic implementation
+         * unit_needs_console() provides, since we want to return false if we are in SERVICE_EXITED state. */
+
+        if (!exec_context_may_touch_console(&s->exec_context))
+                return false;
+
+        return IN_SET(s->state,
+                      SERVICE_START_PRE,
+                      SERVICE_START,
+                      SERVICE_START_POST,
+                      SERVICE_RUNNING,
+                      SERVICE_RELOAD,
+                      SERVICE_STOP,
+                      SERVICE_STOP_SIGABRT,
+                      SERVICE_STOP_SIGTERM,
+                      SERVICE_STOP_SIGKILL,
+                      SERVICE_STOP_POST,
+                      SERVICE_FINAL_SIGTERM,
+                      SERVICE_FINAL_SIGKILL);
+}
+
 static const char* const service_restart_table[_SERVICE_RESTART_MAX] = {
         [SERVICE_RESTART_NO] = "no",
         [SERVICE_RESTART_ON_SUCCESS] = "on-success",
@@ -3921,6 +3927,7 @@ const UnitVTable service_vtable = {
         .bus_commit_properties = bus_service_commit_properties,
 
         .get_timeout = service_get_timeout,
+        .needs_console = service_needs_console,
         .can_transient = true,
 
         .status_message_formats = {
diff --git a/src/core/unit.c b/src/core/unit.c
index 97585a7179..932f05baa2 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -630,6 +630,9 @@ void unit_free(Unit *u) {
         if (u->in_cgroup_empty_queue)
                 LIST_REMOVE(cgroup_empty_queue, u->manager->cgroup_empty_queue, u);
 
+        if (u->on_console)
+                manager_unref_console(u->manager);
+
         unit_release_cgroup(u);
 
         if (!MANAGER_IS_RELOADING(u->manager))
@@ -2305,6 +2308,23 @@ finish:
 
 }
 
+static void unit_update_on_console(Unit *u) {
+        bool b;
+
+        assert(u);
+
+        b = unit_needs_console(u);
+        if (u->on_console == b)
+                return;
+
+        u->on_console = b;
+        if (b)
+                manager_ref_console(u->manager);
+        else
+                manager_unref_console(u->manager);
+
+}
+
 void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_success) {
         Manager *m;
         bool unexpected;
@@ -2345,24 +2365,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su
                 unit_unlink_state_files(u);
         }
 
-        /* Note that this doesn't apply to RemainAfterExit services exiting
-         * successfully, since there's no change of state in that case. Which is
-         * why it is handled in service_set_state() */
-        if (UNIT_IS_INACTIVE_OR_FAILED(os) != UNIT_IS_INACTIVE_OR_FAILED(ns)) {
-                ExecContext *ec;
-
-                ec = unit_get_exec_context(u);
-                if (ec && exec_context_may_touch_console(ec)) {
-                        if (UNIT_IS_INACTIVE_OR_FAILED(ns)) {
-                                m->n_on_console--;
-
-                                if (m->n_on_console == 0)
-                                        /* unset no_console_output flag, since the console is free */
-                                        m->no_console_output = false;
-                        } else
-                                m->n_on_console++;
-                }
-        }
+        unit_update_on_console(u);
 
         if (u->job) {
                 unexpected = false;
@@ -5350,6 +5353,28 @@ void unit_warn_leftover_processes(Unit *u) {
         (void) cg_kill_recursive(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, 0, 0, NULL, log_leftover, u);
 }
 
+bool unit_needs_console(Unit *u) {
+        ExecContext *ec;
+        UnitActiveState state;
+
+        assert(u);
+
+        state = unit_active_state(u);
+
+        if (UNIT_IS_INACTIVE_OR_FAILED(state))
+                return false;
+
+        if (UNIT_VTABLE(u)->needs_console)
+                return UNIT_VTABLE(u)->needs_console(u);
+
+        /* If this unit type doesn't implement this call, let's use a generic fallback implementation: */
+        ec = unit_get_exec_context(u);
+        if (!ec)
+                return false;
+
+        return exec_context_may_touch_console(ec);
+}
+
 static const char* const collect_mode_table[_COLLECT_MODE_MAX] = {
         [COLLECT_INACTIVE] = "inactive",
         [COLLECT_INACTIVE_OR_FAILED] = "inactive-or-failed",
diff --git a/src/core/unit.h b/src/core/unit.h
index 4cd0706676..8c79d4ed2e 100644
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -340,6 +340,7 @@ struct Unit {
         bool sent_dbus_new_signal:1;
 
         bool in_audit:1;
+        bool on_console:1;
 
         bool cgroup_realized:1;
         bool cgroup_members_mask_valid:1;
@@ -541,6 +542,9 @@ struct UnitVTable {
         /* Returns the main PID if there is any defined, or 0. */
         pid_t (*control_pid)(Unit *u);
 
+        /* Returns true if the unit currently needs access to the console */
+        bool (*needs_console)(Unit *u);
+
         /* This is called for each unit type and should be used to
          * enumerate existing devices and load them. However,
          * everything that is loaded here should still stay in
@@ -795,6 +799,8 @@ int unit_prepare_exec(Unit *u);
 
 void unit_warn_leftover_processes(Unit *u);
 
+bool unit_needs_console(Unit *u);
+
 /* Macros which append UNIT= or USER_UNIT= to the message */
 
 #define log_unit_full(unit, level, error, ...)                          \