From f997080b4d17a40b59b398e4354b6368d9c85f69 Mon Sep 17 00:00:00 2001
From: Ivan Shapovalov <intelfx100@gmail.com>
Date: Sat, 7 Mar 2015 08:44:52 -0500
Subject: [PATCH] core: do not spawn jobs or touch other units during
coldplugging
Because the order of coldplugging is not defined, we can reference a
not-yet-coldplugged unit and read its state while it has not yet been
set to a meaningful value.
This way, already active units may get started again.
We fix this by deferring such actions until all units have been at
least somehow coldplugged.
Fixes https://bugs.freedesktop.org/show_bug.cgi?id=88401
(cherry picked from commit 6e392c9c45643d106673c6643ac8bf4e65da13c1)
---
src/core/automount.c | 2 +-
src/core/busname.c | 2 +-
src/core/device.c | 2 +-
src/core/manager.c | 35 +++++++++++++++++++++++++++++++++--
src/core/mount.c | 2 +-
src/core/path.c | 14 ++++++++++----
src/core/scope.c | 2 +-
src/core/service.c | 2 +-
src/core/slice.c | 2 +-
src/core/snapshot.c | 2 +-
src/core/socket.c | 2 +-
src/core/swap.c | 2 +-
src/core/target.c | 2 +-
src/core/timer.c | 14 ++++++++++----
src/core/unit.c | 25 ++++++++++++++++---------
src/core/unit.h | 12 +++++++++---
16 files changed, 89 insertions(+), 33 deletions(-)
diff --git a/src/core/automount.c b/src/core/automount.c
index 9f6bd84..e4c7941 100644
--- a/src/core/automount.c
+++ b/src/core/automount.c
@@ -235,7 +235,7 @@ static void automount_set_state(Automount *a, AutomountState state) {
unit_notify(UNIT(a), state_translation_table[old_state], state_translation_table[state], true);
}
-static int automount_coldplug(Unit *u) {
+static int automount_coldplug(Unit *u, Hashmap *deferred_work) {
Automount *a = AUTOMOUNT(u);
int r;
diff --git a/src/core/busname.c b/src/core/busname.c
index 1d77292..43d7607 100644
--- a/src/core/busname.c
+++ b/src/core/busname.c
@@ -335,7 +335,7 @@ static void busname_set_state(BusName *n, BusNameState state) {
unit_notify(UNIT(n), state_translation_table[old_state], state_translation_table[state], true);
}
-static int busname_coldplug(Unit *u) {
+static int busname_coldplug(Unit *u, Hashmap *deferred_work) {
BusName *n = BUSNAME(u);
int r;
diff --git a/src/core/device.c b/src/core/device.c
index 1cc103c..4ff8827 100644
--- a/src/core/device.c
+++ b/src/core/device.c
@@ -142,7 +142,7 @@ static void device_set_state(Device *d, DeviceState state) {
unit_notify(UNIT(d), state_translation_table[old_state], state_translation_table[state], true);
}
-static int device_coldplug(Unit *u) {
+static int device_coldplug(Unit *u, Hashmap *deferred_work) {
Device *d = DEVICE(u);
assert(d);
diff --git a/src/core/manager.c b/src/core/manager.c
index bc9b7ec..203a6a0 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -983,7 +983,28 @@ static int manager_coldplug(Manager *m) {
Unit *u;
char *k;
- assert(m);
+ /*
+ * Some unit types tend to spawn jobs or check other units' state
+ * during coldplug. This is wrong because it is undefined whether the
+ * units in question have been already coldplugged (i. e. their state
+ * restored). This way, we can easily re-start an already started unit
+ * or otherwise make a wrong decision based on the unit's state.
+ *
+ * Solve this by providing a way for coldplug functions to defer
+ * such actions until after all units have been coldplugged.
+ *
+ * We store Unit* -> int(*)(Unit*).
+ *
+ * https://bugs.freedesktop.org/show_bug.cgi?id=88401
+ */
+ _cleanup_hashmap_free_ Hashmap *deferred_work = NULL;
+ int(*proc)(Unit*);
+
+ assert(m);
+
+ deferred_work = hashmap_new(&trivial_hash_ops);
+ if (!deferred_work)
+ return -ENOMEM;
/* Then, let's set up their initial state. */
HASHMAP_FOREACH_KEY(u, k, m->units, i) {
@@ -993,7 +1014,17 @@ static int manager_coldplug(Manager *m) {
if (u->id != k)
continue;
- q = unit_coldplug(u);
+ q = unit_coldplug(u, deferred_work);
+ if (q < 0)
+ r = q;
+ }
+
+ /* After coldplugging and setting up initial state of the units,
+ * let's perform operations which spawn jobs or query units' state. */
+ HASHMAP_FOREACH_KEY(proc, u, deferred_work, i) {
+ int q;
+
+ q = proc(u);
if (q < 0)
r = q;
}
diff --git a/src/core/mount.c b/src/core/mount.c
index c971330..3ae0eb4 100644
--- a/src/core/mount.c
+++ b/src/core/mount.c
@@ -617,7 +617,7 @@ static void mount_set_state(Mount *m, MountState state) {
m->reload_result = MOUNT_SUCCESS;
}
-static int mount_coldplug(Unit *u) {
+static int mount_coldplug(Unit *u, Hashmap *deferred_work) {
Mount *m = MOUNT(u);
MountState new_state = MOUNT_DEAD;
int r;
diff --git a/src/core/path.c b/src/core/path.c
index e5ea79f..51e36fa 100644
--- a/src/core/path.c
+++ b/src/core/path.c
@@ -440,7 +440,12 @@ static void path_set_state(Path *p, PathState state) {
static void path_enter_waiting(Path *p, bool initial, bool recheck);
-static int path_coldplug(Unit *u) {
+static int path_enter_waiting_coldplug(Unit *u) {
+ path_enter_waiting(PATH(u), true, true);
+ return 0;
+}
+
+static int path_coldplug(Unit *u, Hashmap *deferred_work) {
Path *p = PATH(u);
assert(p);
@@ -449,9 +454,10 @@ static int path_coldplug(Unit *u) {
if (p->deserialized_state != p->state) {
if (p->deserialized_state == PATH_WAITING ||
- p->deserialized_state == PATH_RUNNING)
- path_enter_waiting(p, true, true);
- else
+ p->deserialized_state == PATH_RUNNING) {
+ hashmap_put(deferred_work, u, &path_enter_waiting_coldplug);
+ path_set_state(p, PATH_WAITING);
+ } else
path_set_state(p, p->deserialized_state);
}
diff --git a/src/core/scope.c b/src/core/scope.c
index b41db78..ae6614f 100644
--- a/src/core/scope.c
+++ b/src/core/scope.c
@@ -173,7 +173,7 @@ static int scope_load(Unit *u) {
return scope_verify(s);
}
-static int scope_coldplug(Unit *u) {
+static int scope_coldplug(Unit *u, Hashmap *deferred_work) {
Scope *s = SCOPE(u);
int r;
diff --git a/src/core/service.c b/src/core/service.c
index 15e29be..7781b4e 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -879,7 +879,7 @@ static void service_set_state(Service *s, ServiceState state) {
s->reload_result = SERVICE_SUCCESS;
}
-static int service_coldplug(Unit *u) {
+static int service_coldplug(Unit *u, Hashmap *deferred_work) {
Service *s = SERVICE(u);
int r;
diff --git a/src/core/slice.c b/src/core/slice.c
index ae9819d..61ff9d3 100644
--- a/src/core/slice.c
+++ b/src/core/slice.c
@@ -153,7 +153,7 @@ static int slice_load(Unit *u) {
return slice_verify(s);
}
-static int slice_coldplug(Unit *u) {
+static int slice_coldplug(Unit *u, Hashmap *deferred_work) {
Slice *t = SLICE(u);
assert(t);
diff --git a/src/core/snapshot.c b/src/core/snapshot.c
index b70c3be..b1d8448 100644
--- a/src/core/snapshot.c
+++ b/src/core/snapshot.c
@@ -75,7 +75,7 @@ static int snapshot_load(Unit *u) {
return 0;
}
-static int snapshot_coldplug(Unit *u) {
+static int snapshot_coldplug(Unit *u, Hashmap *deferred_work) {
Snapshot *s = SNAPSHOT(u);
assert(s);
diff --git a/src/core/socket.c b/src/core/socket.c
index 88aae48..760de02 100644
--- a/src/core/socket.c
+++ b/src/core/socket.c
@@ -1326,7 +1326,7 @@ static void socket_set_state(Socket *s, SocketState state) {
unit_notify(UNIT(s), state_translation_table[old_state], state_translation_table[state], true);
}
-static int socket_coldplug(Unit *u) {
+static int socket_coldplug(Unit *u, Hashmap *deferred_work) {
Socket *s = SOCKET(u);
int r;
diff --git a/src/core/swap.c b/src/core/swap.c
index 5c19af5..369abf0 100644
--- a/src/core/swap.c
+++ b/src/core/swap.c
@@ -513,7 +513,7 @@ static void swap_set_state(Swap *s, SwapState state) {
job_add_to_run_queue(UNIT(other)->job);
}
-static int swap_coldplug(Unit *u) {
+static int swap_coldplug(Unit *u, Hashmap *deferred_work) {
Swap *s = SWAP(u);
SwapState new_state = SWAP_DEAD;
int r;
diff --git a/src/core/target.c b/src/core/target.c
index 33fb66b..2411a8e 100644
--- a/src/core/target.c
+++ b/src/core/target.c
@@ -107,7 +107,7 @@ static int target_load(Unit *u) {
return 0;
}
-static int target_coldplug(Unit *u) {
+static int target_coldplug(Unit *u, Hashmap *deferred_work) {
Target *t = TARGET(u);
assert(t);
diff --git a/src/core/timer.c b/src/core/timer.c
index 45744c7..48cf9c1 100644
--- a/src/core/timer.c
+++ b/src/core/timer.c
@@ -268,7 +268,12 @@ static void timer_set_state(Timer *t, TimerState state) {
static void timer_enter_waiting(Timer *t, bool initial);
-static int timer_coldplug(Unit *u) {
+static int timer_enter_waiting_coldplug(Unit *u) {
+ timer_enter_waiting(TIMER(u), false);
+ return 0;
+}
+
+static int timer_coldplug(Unit *u, Hashmap *deferred_work) {
Timer *t = TIMER(u);
assert(t);
@@ -276,9 +281,10 @@ static int timer_coldplug(Unit *u) {
if (t->deserialized_state != t->state) {
- if (t->deserialized_state == TIMER_WAITING)
- timer_enter_waiting(t, false);
- else
+ if (t->deserialized_state == TIMER_WAITING) {
+ hashmap_put(deferred_work, u, &timer_enter_waiting_coldplug);
+ timer_set_state(t, TIMER_WAITING);
+ } else
timer_set_state(t, t->deserialized_state);
}
diff --git a/src/core/unit.c b/src/core/unit.c
index a6558ee..565455b 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -2859,27 +2859,34 @@ int unit_add_node_link(Unit *u, const char *what, bool wants) {
return 0;
}
-int unit_coldplug(Unit *u) {
+static int unit_add_deserialized_job_coldplug(Unit *u) {
+ int r;
+
+ r = manager_add_job(u->manager, u->deserialized_job, u, JOB_IGNORE_REQUIREMENTS, false, NULL, NULL);
+ if (r < 0)
+ return r;
+
+ u->deserialized_job = _JOB_TYPE_INVALID;
+
+ return 0;
+}
+
+int unit_coldplug(Unit *u, Hashmap *deferred_work) {
int r;
assert(u);
if (UNIT_VTABLE(u)->coldplug)
- if ((r = UNIT_VTABLE(u)->coldplug(u)) < 0)
+ if ((r = UNIT_VTABLE(u)->coldplug(u, deferred_work)) < 0)
return r;
if (u->job) {
r = job_coldplug(u->job);
if (r < 0)
return r;
- } else if (u->deserialized_job >= 0) {
+ } else if (u->deserialized_job >= 0)
/* legacy */
- r = manager_add_job(u->manager, u->deserialized_job, u, JOB_IGNORE_REQUIREMENTS, false, NULL, NULL);
- if (r < 0)
- return r;
-
- u->deserialized_job = _JOB_TYPE_INVALID;
- }
+ hashmap_put(deferred_work, u, &unit_add_deserialized_job_coldplug);
return 0;
}
diff --git a/src/core/unit.h b/src/core/unit.h
index 291bc77..7ebc489 100644
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -307,8 +307,14 @@ struct UnitVTable {
int (*load)(Unit *u);
/* If a lot of units got created via enumerate(), this is
- * where to actually set the state and call unit_notify(). */
- int (*coldplug)(Unit *u);
+ * where to actually set the state and call unit_notify().
+ *
+ * This must not reference other units (maybe implicitly through spawning
+ * jobs), because it is possible that they are not yet coldplugged.
+ * Such actions must be deferred until the end of coldplug bу adding
+ * a "Unit* -> int(*)(Unit*)" entry into the hashmap.
+ */
+ int (*coldplug)(Unit *u, Hashmap *deferred_work);
void (*dump)(Unit *u, FILE *f, const char *prefix);
@@ -544,7 +550,7 @@ int unit_deserialize(Unit *u, FILE *f, FDSet *fds);
int unit_add_node_link(Unit *u, const char *what, bool wants);
-int unit_coldplug(Unit *u);
+int unit_coldplug(Unit *u, Hashmap *deferred_work);
void unit_status_printf(Unit *u, const char *status, const char *unit_status_msg_format) _printf_(3, 0);