From 8559b3b75cbd04ec132f55bdb61eae7faf27d6fc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 7 Feb 2018 14:52:22 +0100 Subject: [PATCH] core: rework how we connect to the bus This removes the current bus_init() call, as it had multiple problems: it munged handling of the three bus connections we care about (private, "api" and system) into one, even though the conditions when which was ready are very different. It also added redundant logging, as the individual calls it called all logged on their own anyway. The three calls bus_init_api(), bus_init_private() and bus_init_system() are now made public. A new call manager_dbus_is_running() is added that works much like manager_journal_is_running() and is a lot more careful when checking whether dbus is around. Optionally it checks the unit's deserialized_state rather than state, in order to accomodate for cases where we cant to connect to the bus before deserializing the "subscribed" list, before coldplugging the units. manager_recheck_dbus() is added, that works a lot like manager_recheck_journal() and is invoked in unit_notify(), i.e. when units change state. All in all this should make handling a bit more alike to journal handling, and it also fixes one major bug: when running in user mode we'll now connect to the system bus early on, without conditionalizing this in anyway. --- src/core/dbus.c | 26 ++--------- src/core/dbus.h | 4 +- src/core/manager.c | 111 +++++++++++++++++++++++++++++++-------------- src/core/manager.h | 1 + src/core/unit.c | 28 ++++-------- 5 files changed, 93 insertions(+), 77 deletions(-) diff --git a/src/core/dbus.c b/src/core/dbus.c index c46c299207..65079135df 100644 --- a/src/core/dbus.c +++ b/src/core/dbus.c @@ -846,7 +846,7 @@ static int bus_setup_api(Manager *m, sd_bus *bus) { return 0; } -static int bus_init_api(Manager *m) { +int bus_init_api(Manager *m) { _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; int r; @@ -907,7 +907,7 @@ static int bus_setup_system(Manager *m, sd_bus *bus) { return 0; } -static int bus_init_system(Manager *m) { +int bus_init_system(Manager *m) { _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; int r; @@ -942,7 +942,7 @@ static int bus_init_system(Manager *m) { return 0; } -static int bus_init_private(Manager *m) { +int bus_init_private(Manager *m) { _cleanup_close_ int fd = -1; union sockaddr_union sa = { .un.sun_family = AF_UNIX @@ -1014,26 +1014,6 @@ static int bus_init_private(Manager *m) { return 0; } -int bus_init(Manager *m, bool try_bus_connect) { - int r; - - if (try_bus_connect) { - r = bus_init_system(m); - if (r < 0) - return log_error_errno(r, "Failed to initialize D-Bus connection: %m"); - - r = bus_init_api(m); - if (r < 0) - return log_error_errno(r, "Error occured during D-Bus APIs initialization: %m"); - } - - r = bus_init_private(m); - if (r < 0) - return log_error_errno(r, "Failed to create private D-Bus server: %m"); - - return 0; -} - static void destroy_bus(Manager *m, sd_bus **bus) { Iterator i; Unit *u; diff --git a/src/core/dbus.h b/src/core/dbus.h index 17dfbc9f97..fb4988dace 100644 --- a/src/core/dbus.h +++ b/src/core/dbus.h @@ -24,7 +24,9 @@ int bus_send_queued_message(Manager *m); -int bus_init(Manager *m, bool try_bus_connect); +int bus_init_private(Manager *m); +int bus_init_api(Manager *m); +int bus_init_system(Manager *m); void bus_done_private(Manager *m); void bus_done_api(Manager *m); diff --git a/src/core/manager.c b/src/core/manager.c index b54c021c75..8ae70b648c 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -985,26 +985,6 @@ static int manager_setup_user_lookup_fd(Manager *m) { return 0; } -static int manager_connect_bus(Manager *m, bool reexecuting) { - bool try_bus_connect; - Unit *u = NULL; - - assert(m); - - if (m->test_run_flags) - return 0; - - u = manager_get_unit(m, SPECIAL_DBUS_SERVICE); - - try_bus_connect = - (u && SERVICE(u)->deserialized_state == SERVICE_RUNNING) && - (reexecuting || - (MANAGER_IS_USER(m) && getenv("DBUS_SESSION_BUS_ADDRESS"))); - - /* Try to connect to the buses, if possible. */ - return bus_init(m, try_bus_connect); -} - static unsigned manager_dispatch_cleanup_queue(Manager *m) { Unit *u; unsigned n = 0; @@ -1374,6 +1354,38 @@ static void manager_distribute_fds(Manager *m, FDSet *fds) { } } +static bool manager_dbus_is_running(Manager *m, bool deserialized) { + Unit *u; + + assert(m); + + /* This checks whether the dbus instance we are supposed to expose our APIs on is up. We check both the socket + * and the service unit. If the 'deserialized' parameter is true we'll check the deserialized state of the unit + * rather than the current one. */ + + if (m->test_run_flags != 0) + return false; + + /* If we are in the user instance, and the env var is already set for us, then this means D-Bus is ran + * somewhere outside of our own logic. Let's use it */ + if (MANAGER_IS_USER(m) && getenv("DBUS_SESSION_BUS_ADDRESS")) + return true; + + u = manager_get_unit(m, SPECIAL_DBUS_SOCKET); + if (!u) + return false; + if ((deserialized ? SOCKET(u)->deserialized_state : SOCKET(u)->state) != SOCKET_RUNNING) + return false; + + u = manager_get_unit(m, SPECIAL_DBUS_SERVICE); + if (!u) + return false; + if (!IN_SET((deserialized ? SERVICE(u)->deserialized_state : SERVICE(u)->state), SERVICE_RUNNING, SERVICE_RELOAD)) + return false; + + return true; +} + int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { int r; @@ -1454,9 +1466,22 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { /* This shouldn't fail, except if things are really broken. */ return r; - /* Let's connect to the bus now. */ - (void) manager_connect_bus(m, !!serialization); + /* Let's set up our private bus connection now, unconditionally */ + (void) bus_init_private(m); + /* If we are in --user mode also connect to the system bus now */ + if (MANAGER_IS_USER(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(m, !!serialization)) { + (void) bus_init_api(m); + + if (MANAGER_IS_SYSTEM(m)) + (void) bus_init_system(m); + } + + /* Now that we are connected to all possible busses, let's deserialize who is tracking us. */ (void) bus_track_coldplug(m, &m->subscribed, false, m->deserialized_subscribed); m->deserialized_subscribed = strv_free(m->deserialized_subscribed); @@ -2295,23 +2320,21 @@ static int manager_dispatch_signal_fd(sd_event_source *source, int fd, uint32_t /* This is a nop on non-init */ break; - case SIGUSR1: { - Unit *u; + case SIGUSR1: - u = manager_get_unit(m, SPECIAL_DBUS_SERVICE); - - if (!u || UNIT_IS_ACTIVE_OR_RELOADING(unit_active_state(u))) { + if (manager_dbus_is_running(m, false)) { log_info("Trying to reconnect to bus..."); - bus_init(m, true); - } - if (!u || !UNIT_IS_ACTIVE_OR_ACTIVATING(unit_active_state(u))) { - log_info("Loading D-Bus service..."); + (void) bus_init_api(m); + + if (MANAGER_IS_SYSTEM(m)) + (void) bus_init_system(m); + } else { + log_info("Starting D-Bus service..."); manager_start_target(m, SPECIAL_DBUS_SERVICE, JOB_REPLACE); } break; - } case SIGUSR2: { _cleanup_free_ char *dump = NULL; @@ -3154,8 +3177,9 @@ int manager_reload(Manager *m) { exec_runtime_vacuum(m); - /* It might be safe to log to the journal now. */ + /* It might be safe to log to the journal now and connect to dbus */ manager_recheck_journal(m); + manager_recheck_dbus(m); /* Sync current state of bus names with our set of listening units */ if (m->api_bus) @@ -3519,6 +3543,27 @@ int manager_set_default_rlimits(Manager *m, struct rlimit **default_rlimit) { return 0; } +void manager_recheck_dbus(Manager *m) { + assert(m); + + /* Connects to the bus if the dbus service and socket are running. If we are running in user mode this is all + * it does. In system mode we'll also connect to the system bus (which will most likely just reuse the + * connection of the API bus). That's because the system bus after all runs as service of the system instance, + * while in the user instance we can assume it's already there. */ + + if (manager_dbus_is_running(m, false)) { + (void) bus_init_api(m); + + if (MANAGER_IS_SYSTEM(m)) + (void) bus_init_system(m); + } else { + (void) bus_done_api(m); + + if (MANAGER_IS_SYSTEM(m)) + (void) bus_done_system(m); + } +} + static bool manager_journal_is_running(Manager *m) { Unit *u; diff --git a/src/core/manager.h b/src/core/manager.h index fc70593def..b731e6e8f1 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -425,6 +425,7 @@ bool manager_unit_inactive_or_pending(Manager *m, const char *name); void manager_check_finished(Manager *m); +void manager_recheck_dbus(Manager *m); void manager_recheck_journal(Manager *m); void manager_set_show_status(Manager *m, ShowStatus mode); diff --git a/src/core/unit.c b/src/core/unit.c index 0ad7dc108b..47a06e4297 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2326,18 +2326,16 @@ static void unit_update_on_console(Unit *u) { } void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_success) { - Manager *m; bool unexpected; + Manager *m; assert(u); assert(os < _UNIT_ACTIVE_STATE_MAX); assert(ns < _UNIT_ACTIVE_STATE_MAX); - /* Note that this is called for all low-level state changes, - * even if they might map to the same high-level - * UnitActiveState! That means that ns == os is an expected - * behavior here. For example: if a mount point is remounted - * this function will be called too! */ + /* Note that this is called for all low-level state changes, even if they might map to the same high-level + * UnitActiveState! That means that ns == os is an expected behavior here. For example: if a mount point is + * remounted this function will be called too! */ m = u->manager; @@ -2463,12 +2461,6 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su /* Some names are special */ if (UNIT_IS_ACTIVE_OR_RELOADING(ns)) { - if (unit_has_name(u, SPECIAL_DBUS_SERVICE)) - /* The bus might have just become available, - * hence try to connect to it, if we aren't - * yet connected. */ - bus_init(m, true); - if (u->type == UNIT_SERVICE && !UNIT_IS_ACTIVE_OR_RELOADING(os) && !MANAGER_IS_RELOADING(m)) { @@ -2481,8 +2473,6 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su manager_send_unit_plymouth(m, u); } else { - /* We don't care about D-Bus going down here, since we'll get an asynchronous notification for it - * anyway. */ if (UNIT_IS_INACTIVE_OR_FAILED(ns) && !UNIT_IS_INACTIVE_OR_FAILED(os) @@ -2512,17 +2502,15 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su } manager_recheck_journal(m); + manager_recheck_dbus(m); unit_trigger_notify(u); if (!MANAGER_IS_RELOADING(u->manager)) { - /* Maybe we finished startup and are now ready for - * being stopped because unneeded? */ + /* Maybe we finished startup and are now ready for being stopped because unneeded? */ unit_check_unneeded(u); - /* Maybe we finished startup, but something we needed - * has vanished? Let's die then. (This happens when - * something BindsTo= to a Type=oneshot unit, as these - * units go directly from starting to inactive, + /* Maybe we finished startup, but something we needed has vanished? Let's die then. (This happens when + * something BindsTo= to a Type=oneshot unit, as these units go directly from starting to inactive, * without ever entering started.) */ unit_check_binds_to(u);