From bee6e755bb8e53a7a436e221b015ce0232ed87c0 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 10 May 2023 13:54:15 +0800 Subject: core: only refuse Type=dbus service enqueuing if dbus has stop job Follow-up for #27579 In #27579 we refused all StartUnit requests for Type=dbus units if dbus is not running, which means if dbus is manually stopped, user can't use systemctl to start Type=dbus units again, which is incorrect. The only culprit that leads to the cancellation of the whole transaction mentioned in #26799 is job type conflict on dbus. So let's relax the restriction and only refuse job enqueuing if dbus has a stop job. To summarize, the case we want to avoid is: 1. dbus has a stop job installed 2. StartUnit/ActivationRequest is received 3. Type=dbus service gets started, which has Requires=dbus.socket 4. dbus is pulled in again, resulting in job type conflict What we can support is: 1. dbus is already stopped 2. StartUnit is received (possibly through systemctl, i.e. on private bus) 3. Type=dbus service gets started, which will wait for dbus to start 4. dbus is started again, thus the job for Type=dbus service Replaces #27590 Fixes #27588 --- src/core/dbus-unit.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) (limited to 'src/core') diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 5b89c76586..59d541ebfe 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -1875,13 +1875,30 @@ int bus_unit_queue_job( (type == JOB_STOP && u->refuse_manual_stop) || (IN_SET(type, JOB_RESTART, JOB_TRY_RESTART) && (u->refuse_manual_start || u->refuse_manual_stop)) || (type == JOB_RELOAD_OR_START && job_type_collapse(type, u) == JOB_START && u->refuse_manual_start)) - return sd_bus_error_setf(error, BUS_ERROR_ONLY_BY_DEPENDENCY, "Operation refused, unit %s may be requested by dependency only (it is configured to refuse manual start/stop).", u->id); - - /* dbus-broker issues StartUnit for activation requests, so let's apply the same check - * used in signal_activation_request(). */ - if (type == JOB_START && u->type == UNIT_SERVICE && - SERVICE(u)->type == SERVICE_DBUS && !manager_dbus_is_running(u->manager)) - return sd_bus_error_set(error, BUS_ERROR_SHUTTING_DOWN, "Refusing activation, D-Bus is not running."); + return sd_bus_error_setf(error, + BUS_ERROR_ONLY_BY_DEPENDENCY, + "Operation refused, unit %s may be requested by dependency only (it is configured to refuse manual start/stop).", + u->id); + + /* dbus-broker issues StartUnit for activation requests, and Type=dbus services automatically + * gain dependency on dbus.socket. Therefore, if dbus has a pending stop job, the new start + * job that pulls in dbus again would cause job type conflict. Let's avoid that by rejecting + * job enqueuing early. + * + * Note that unlike signal_activation_request(), we can't use unit_inactive_or_pending() + * here. StartUnit is a more generic interface, and thus users are allowed to use e.g. systemctl + * to start Type=dbus services even when dbus is inactive. */ + if (type == JOB_START && u->type == UNIT_SERVICE && SERVICE(u)->type == SERVICE_DBUS) + FOREACH_STRING(dbus_unit, SPECIAL_DBUS_SOCKET, SPECIAL_DBUS_SERVICE) { + Unit *dbus; + + dbus = manager_get_unit(u->manager, dbus_unit); + if (dbus && unit_stop_pending(dbus)) + return sd_bus_error_setf(error, + BUS_ERROR_SHUTTING_DOWN, + "Operation for unit %s refused, D-Bus is shutting down.", + u->id); + } r = sd_bus_message_new_method_return(message, &reply); if (r < 0) -- cgit v1.2.1 From 2b680534c9667341551b39f4cc9735cd6e8c014e Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 11 May 2023 18:55:43 +0800 Subject: Revert "core/manager: export manager_dbus_is_running" and partially "core: refuse dbus activation if dbus is not running" This reverts commit e8863150653931ae2ffc91757623f179ce763628 and partially 53964fd26b4a01191609ffc064aa8ccccd28e377. Specifically, changes to signal_activation_request() is not desired. --- src/core/dbus.c | 5 +++-- src/core/manager.c | 8 ++++---- src/core/manager.h | 5 ----- 3 files changed, 7 insertions(+), 11 deletions(-) (limited to 'src/core') diff --git a/src/core/dbus.c b/src/core/dbus.c index 2f5feeaec3..c41e1a6c74 100644 --- a/src/core/dbus.c +++ b/src/core/dbus.c @@ -160,8 +160,9 @@ static int signal_activation_request(sd_bus_message *message, void *userdata, sd return 0; } - if (!manager_dbus_is_running(m)) { - r = sd_bus_error_set(&error, BUS_ERROR_SHUTTING_DOWN, "Refusing activation, D-Bus is not running."); + if (manager_unit_inactive_or_pending(m, SPECIAL_DBUS_SOCKET) || + manager_unit_inactive_or_pending(m, SPECIAL_DBUS_SERVICE)) { + r = sd_bus_error_set(&error, BUS_ERROR_SHUTTING_DOWN, "Refusing activation, D-Bus is shutting down."); goto failed; } diff --git a/src/core/manager.c b/src/core/manager.c index ec8851ca22..36881de479 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1801,7 +1801,7 @@ static void manager_distribute_fds(Manager *m, FDSet *fds) { } } -bool manager_dbus_is_running_full(Manager *m, bool deserialized) { +static bool manager_dbus_is_running(Manager *m, bool deserialized) { Unit *u; assert(m); @@ -1843,7 +1843,7 @@ static void manager_setup_bus(Manager *m) { (void) bus_init_system(m); /* Let's connect to the bus now, but only if the unit is supposed to be up */ - if (manager_dbus_is_running_full(m, MANAGER_IS_RELOADING(m))) { + if (manager_dbus_is_running(m, MANAGER_IS_RELOADING(m))) { (void) bus_init_api(m); if (MANAGER_IS_SYSTEM(m)) @@ -2935,7 +2935,7 @@ static int manager_dispatch_signal_fd(sd_event_source *source, int fd, uint32_t break; case SIGUSR1: - if (manager_dbus_is_running(m)) { + if (manager_dbus_is_running(m, false)) { log_info("Trying to reconnect to bus..."); (void) bus_init_api(m); @@ -4140,7 +4140,7 @@ void manager_recheck_dbus(Manager *m) { if (MANAGER_IS_RELOADING(m)) return; /* don't check while we are reloading… */ - if (manager_dbus_is_running(m)) { + if (manager_dbus_is_running(m, false)) { (void) bus_init_api(m); if (MANAGER_IS_SYSTEM(m)) diff --git a/src/core/manager.h b/src/core/manager.h index 951d46222e..486eda11b6 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -504,11 +504,6 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(Manager*, manager_free); int manager_startup(Manager *m, FILE *serialization, FDSet *fds, const char *root); -bool manager_dbus_is_running_full(Manager *m, bool deserialized); -static inline bool manager_dbus_is_running(Manager *m) { - return manager_dbus_is_running_full(m, false); -} - Job *manager_get_job(Manager *m, uint32_t id); Unit *manager_get_unit(Manager *m, const char *name); -- cgit v1.2.1