From 5a69973ff241dcefbd3bd12827dd43088e75ed4f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Jan 2018 19:52:14 +0100 Subject: [PATCH 1/4] manager: add some explanatory comments to manager_dispatch_idle_pipe_fd() --- src/core/manager.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/core/manager.c b/src/core/manager.c index 4130b249729..f764fb58873 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; From 46fb617bf94a202c74e753407f28d59bec1339f7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Jan 2018 19:52:29 +0100 Subject: [PATCH 2/4] manager: minor manager_get_show_status() simplification Since the the whole function ultimately is just a fancy getter for the show_status field, let's actually return it as last step literally without an extra needless "if". --- src/core/manager.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index f764fb58873..b58f3451d7a 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -3548,10 +3548,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) { From bb2c7685454842549bc1fe47adc35cbca2a84190 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Jan 2018 19:54:26 +0100 Subject: [PATCH 3/4] core: add a new unit_needs_console() call This call determines whether a specific unit currently needs access to the console. It's a fancy wrapper around exec_context_may_touch_console() ultimately, however for service units we'll explicitly exclude the SERVICE_EXITED state from when we report true. --- src/core/service.c | 27 +++++++++++++++++++++++++++ src/core/unit.c | 22 ++++++++++++++++++++++ src/core/unit.h | 5 +++++ 3 files changed, 54 insertions(+) diff --git a/src/core/service.c b/src/core/service.c index 81c776cd15e..425e2a1a3ce 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -3806,6 +3806,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 +3947,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 97585a7179f..4081db83516 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -5350,6 +5350,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 4cd07066761..ec171ada311 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -541,6 +541,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 +798,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, ...) \ From adefcf2821a386184991054ed2bb6dacc90d7419 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 24 Jan 2018 19:59:55 +0100 Subject: [PATCH 4/4] core: rework how we count the n_on_console counter Let's add a per-unit boolean that tells us whether our unit is currently counted or not. This way it's unlikely we get out of sync again and things are generally more robust. This also allows us to remove the counting logic specific to service units (which was in fact mostly a copy from the generic implementation), in favour of fully generic code. Replaces: #7824 --- src/core/manager.c | 15 +++++++++++++++ src/core/manager.h | 3 +++ src/core/service.c | 20 -------------------- src/core/unit.c | 39 +++++++++++++++++++++------------------ src/core/unit.h | 1 + 5 files changed, 40 insertions(+), 38 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index b58f3451d7a..f5c89b04149 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -4072,6 +4072,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 90d5258b539..2bfcbd559b9 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -452,6 +452,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 425e2a1a3ce..6476dc68c50 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)); diff --git a/src/core/unit.c b/src/core/unit.c index 4081db83516..932f05baa27 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; diff --git a/src/core/unit.h b/src/core/unit.h index ec171ada311..8c79d4ed2eb 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;