packages/systemd
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
systemd/0001-core-drop-initial-ListNames-bus-call-from-PID1.patch

332 lines
14 KiB
Diff
Raw Blame History

From fc67a943d989d5e74577adea9676cdc7928b08fc Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Mon, 23 Dec 2019 17:31:34 +0100
Subject: [PATCH] core: drop initial ListNames() bus call from PID 1
Previously, when first connecting to the bus after connecting to it we'd
issue a ListNames() bus call to the driver to figure out which bus names
are currently active. This information was then used to initialize the
initial state for services that use BusName=.
This change removes the whole code for this and replaces it with
something vastly simpler.
First of all, the ListNames() call was issues synchronosuly, which meant
if dbus was for some reason synchronously calling into PID1 for some
reason we'd deadlock. As it turns out there's now a good chance it does:
the nss-systemd userdb hookup means that any user dbus-daemon resolves
might result in a varlink call into PID 1, and dbus resolves quite a lot
of users while parsing its policy. My original goal was to fix this
deadlock.
But as it turns out we don't need the ListNames() call at all anymore,
since #12957 has been merged. That PR was supposed to fix a race where
asynchronous installation of bus matches would cause us missing the
initial owner of a bus name when a service is first started. It fixed it
(correctly) by enquiring with GetOwnerName() who currently owns the
name, right after installing the match. But this means whenever we start watching a bus name we anyway
issue a GetOwnerName() for it, and that means also when first connecting
to the bus we don't need to issue ListNames() anymore since that just
tells us the same info: which names are currently owned.
hence, let's drop ListNames() and instead make better use of the
GetOwnerName() result: if it failed the name is not owned.
Also, while we are at it, let's simplify the unit's owner_name_changed()
callback(): let's drop the "old_owner" argument. We never used that
besides logging, and it's hard to synthesize from just the return of a
GetOwnerName(), hence don't bother.
---
src/core/dbus.c | 112 -----------------------------------------------------
src/core/dbus.h | 2 -
src/core/manager.c | 4 --
src/core/manager.h | 2 -
src/core/service.c | 15 ++-----
src/core/unit.c | 23 ++++++-----
src/core/unit.h | 2 +-
7 files changed, 16 insertions(+), 144 deletions(-)
diff --git a/src/core/dbus.c b/src/core/dbus.c
index cef1789..941219f 100644
--- a/src/core/dbus.c
+++ b/src/core/dbus.c
@@ -719,114 +719,6 @@ static int bus_on_connection(sd_event_source *s, int fd, uint32_t revents, void
return 0;
}
-static int manager_dispatch_sync_bus_names(sd_event_source *es, void *userdata) {
- _cleanup_strv_free_ char **names = NULL;
- Manager *m = userdata;
- const char *name;
- Iterator i;
- Unit *u;
- int r;
-
- assert(es);
- assert(m);
- assert(m->sync_bus_names_event_source == es);
-
- /* First things first, destroy the defer event so that we aren't triggered again */
- m->sync_bus_names_event_source = sd_event_source_unref(m->sync_bus_names_event_source);
-
- /* Let's see if there's anything to do still? */
- if (!m->api_bus)
- return 0;
- if (hashmap_isempty(m->watch_bus))
- return 0;
-
- /* OK, let's sync up the names. Let's see which names are currently on the bus. */
- r = sd_bus_list_names(m->api_bus, &names, NULL);
- if (r < 0)
- return log_error_errno(r, "Failed to get initial list of names: %m");
-
- /* We have to synchronize the current bus names with the
- * list of active services. To do this, walk the list of
- * all units with bus names. */
- HASHMAP_FOREACH_KEY(u, name, m->watch_bus, i) {
- Service *s = SERVICE(u);
-
- assert(s);
-
- if (!streq_ptr(s->bus_name, name)) {
- log_unit_warning(u, "Bus name has changed from %s → %s, ignoring.", s->bus_name, name);
- continue;
- }
-
- /* Check if a service's bus name is in the list of currently
- * active names */
- if (strv_contains(names, name)) {
- _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL;
- const char *unique;
-
- /* If it is, determine its current owner */
- r = sd_bus_get_name_creds(m->api_bus, name, SD_BUS_CREDS_UNIQUE_NAME, &creds);
- if (r < 0) {
- log_full_errno(r == -ENXIO ? LOG_DEBUG : LOG_ERR, r, "Failed to get bus name owner %s: %m", name);
- continue;
- }
-
- r = sd_bus_creds_get_unique_name(creds, &unique);
- if (r < 0) {
- log_full_errno(r == -ENXIO ? LOG_DEBUG : LOG_ERR, r, "Failed to get unique name for %s: %m", name);
- continue;
- }
-
- /* Now, let's compare that to the previous bus owner, and
- * if it's still the same, all is fine, so just don't
- * bother the service. Otherwise, the name has apparently
- * changed, so synthesize a name owner changed signal. */
-
- if (!streq_ptr(unique, s->bus_name_owner))
- UNIT_VTABLE(u)->bus_name_owner_change(u, s->bus_name_owner, unique);
- } else {
- /* So, the name we're watching is not on the bus.
- * This either means it simply hasn't appeared yet,
- * or it was lost during the daemon reload.
- * Check if the service has a stored name owner,
- * and synthesize a name loss signal in this case. */
-
- if (s->bus_name_owner)
- UNIT_VTABLE(u)->bus_name_owner_change(u, s->bus_name_owner, NULL);
- }
- }
-
- return 0;
-}
-
-int manager_enqueue_sync_bus_names(Manager *m) {
- int r;
-
- assert(m);
-
- /* Enqueues a request to synchronize the bus names in a later event loop iteration. The callers generally don't
- * want us to invoke ->bus_name_owner_change() unit calls from their stack frames as this might result in event
- * dispatching on its own creating loops, hence we simply create a defer event for the event loop and exit. */
-
- if (m->sync_bus_names_event_source)
- return 0;
-
- r = sd_event_add_defer(m->event, &m->sync_bus_names_event_source, manager_dispatch_sync_bus_names, m);
- if (r < 0)
- return log_error_errno(r, "Failed to create bus name synchronization event: %m");
-
- r = sd_event_source_set_priority(m->sync_bus_names_event_source, SD_EVENT_PRIORITY_IDLE);
- if (r < 0)
- return log_error_errno(r, "Failed to set event priority: %m");
-
- r = sd_event_source_set_enabled(m->sync_bus_names_event_source, SD_EVENT_ONESHOT);
- if (r < 0)
- return log_error_errno(r, "Failed to set even to oneshot: %m");
-
- (void) sd_event_source_set_description(m->sync_bus_names_event_source, "manager-sync-bus-names");
- return 0;
-}
-
static int bus_setup_api(Manager *m, sd_bus *bus) {
Iterator i;
char *name;
@@ -910,10 +802,6 @@ int bus_init_api(Manager *m) {
m->api_bus = TAKE_PTR(bus);
- r = manager_enqueue_sync_bus_names(m);
- if (r < 0)
- return r;
-
return 0;
}
diff --git a/src/core/dbus.h b/src/core/dbus.h
index f1c0fa8..d5ba653 100644
--- a/src/core/dbus.h
+++ b/src/core/dbus.h
@@ -21,8 +21,6 @@ int bus_fdset_add_all(Manager *m, FDSet *fds);
void bus_track_serialize(sd_bus_track *t, FILE *f, const char *prefix);
int bus_track_coldplug(Manager *m, sd_bus_track **t, bool recursive, char **l);
-int manager_enqueue_sync_bus_names(Manager *m);
-
int bus_foreach_bus(Manager *m, sd_bus_track *subscribed2, int (*send_message)(sd_bus *bus, void *userdata), void *userdata);
int bus_verify_manage_units_async(Manager *m, sd_bus_message *call, sd_bus_error *error);
diff --git a/src/core/manager.c b/src/core/manager.c
index 171ff04..dbd25af 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -1373,7 +1373,6 @@ Manager* manager_free(Manager *m) {
sd_event_source_unref(m->jobs_in_progress_event_source);
sd_event_source_unref(m->run_queue_event_source);
sd_event_source_unref(m->user_lookup_event_source);
- sd_event_source_unref(m->sync_bus_names_event_source);
safe_close(m->signal_fd);
safe_close(m->notify_fd);
@@ -1610,9 +1609,6 @@ static void manager_ready(Manager *m) {
manager_recheck_journal(m);
manager_recheck_dbus(m);
- /* Sync current state of bus names with our set of listening units */
- (void) manager_enqueue_sync_bus_names(m);
-
/* Let's finally catch up with any changes that took place while we were reloading/reexecing */
manager_catchup(m);
diff --git a/src/core/manager.h b/src/core/manager.h
index 51df7f8..8ca8e38 100644
--- a/src/core/manager.h
+++ b/src/core/manager.h
@@ -219,8 +219,6 @@ struct Manager {
int user_lookup_fds[2];
sd_event_source *user_lookup_event_source;
- sd_event_source *sync_bus_names_event_source;
-
UnitFileScope unit_file_scope;
LookupPaths lookup_paths;
Hashmap *unit_id_map;
diff --git a/src/core/service.c b/src/core/service.c
index 49ad166..447c7af 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -4062,24 +4062,17 @@ static int service_get_timeout(Unit *u, usec_t *timeout) {
return 1;
}
-static void service_bus_name_owner_change(
- Unit *u,
- const char *old_owner,
- const char *new_owner) {
+static void service_bus_name_owner_change(Unit *u, const char *new_owner) {
Service *s = SERVICE(u);
int r;
assert(s);
- assert(old_owner || new_owner);
-
- if (old_owner && new_owner)
- log_unit_debug(u, "D-Bus name %s changed owner from %s to %s", s->bus_name, old_owner, new_owner);
- else if (old_owner)
- log_unit_debug(u, "D-Bus name %s no longer registered by %s", s->bus_name, old_owner);
+ if (new_owner)
+ log_unit_debug(u, "D-Bus name %s now owned by %s", s->bus_name, new_owner);
else
- log_unit_debug(u, "D-Bus name %s now registered by %s", s->bus_name, new_owner);
+ log_unit_debug(u, "D-Bus name %s now not owned by anyone.", s->bus_name);
s->bus_name_good = !!new_owner;
diff --git a/src/core/unit.c b/src/core/unit.c
index 8781132..e137acc 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -3185,24 +3185,21 @@ int unit_load_related_unit(Unit *u, const char *type, Unit **_found) {
}
static int signal_name_owner_changed(sd_bus_message *message, void *userdata, sd_bus_error *error) {
- const char *name, *old_owner, *new_owner;
+ const char *new_owner;
Unit *u = userdata;
int r;
assert(message);
assert(u);
- r = sd_bus_message_read(message, "sss", &name, &old_owner, &new_owner);
+ r = sd_bus_message_read(message, "sss", NULL, NULL, &new_owner);
if (r < 0) {
bus_log_parse_error(r);
return 0;
}
- old_owner = empty_to_null(old_owner);
- new_owner = empty_to_null(new_owner);
-
if (UNIT_VTABLE(u)->bus_name_owner_change)
- UNIT_VTABLE(u)->bus_name_owner_change(u, old_owner, new_owner);
+ UNIT_VTABLE(u)->bus_name_owner_change(u, empty_to_null(new_owner));
return 0;
}
@@ -3223,15 +3220,17 @@ static int get_name_owner_handler(sd_bus_message *message, void *userdata, sd_bu
if (!sd_bus_error_has_name(e, "org.freedesktop.DBus.Error.NameHasNoOwner"))
log_unit_error(u, "Unexpected error response from GetNameOwner(): %s", e->message);
- return 0;
- }
+ new_owner = NULL;
+ } else {
+ r = sd_bus_message_read(message, "s", &new_owner);
+ if (r < 0)
+ return bus_log_parse_error(r);
- r = sd_bus_message_read(message, "s", &new_owner);
- if (r < 0)
- return bus_log_parse_error(r);
+ assert(!isempty(new_owner));
+ }
if (UNIT_VTABLE(u)->bus_name_owner_change)
- UNIT_VTABLE(u)->bus_name_owner_change(u, NULL, new_owner);
+ UNIT_VTABLE(u)->bus_name_owner_change(u, new_owner);
return 0;
}
diff --git a/src/core/unit.h b/src/core/unit.h
index c5d8170..4410014 100644
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -530,7 +530,7 @@ typedef struct UnitVTable {
void (*notify_message)(Unit *u, const struct ucred *ucred, char **tags, FDSet *fds);
/* Called whenever a name this Unit registered for comes or goes away. */
- void (*bus_name_owner_change)(Unit *u, const char *old_owner, const char *new_owner);
+ void (*bus_name_owner_change)(Unit *u, const char *new_owner);
/* Called for each property that is being set */
int (*bus_set_property)(Unit *u, const char *name, sd_bus_message *message, UnitWriteFlags flags, sd_bus_error *error);
--
1.8.3.1
Reference in New Issue View Git Blame Copy Permalink