From: Ivan Shapovalov <intelfx100@gmail.com>
Date: Sat, 7 Mar 2015 13:44:52 +0000 (-0500)
Subject: core: do not spawn jobs or touch other units during coldplugging
X-Git-Tag: v219.0~482
X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git?p=elogind.git;a=commitdiff_plain;h=6e392c9c45643d106673c6643ac8bf4e65da13c1

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
---

diff --git a/src/core/automount.c b/src/core/automount.c
index 4a509efaf..0539fbbc4 100644
--- a/src/core/automount.c
+++ b/src/core/automount.c
@@ -233,7 +233,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 1d77292f9..43d7607a3 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 eb976b860..6b489a4c9 100644
--- a/src/core/device.c
+++ b/src/core/device.c
@@ -140,7 +140,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 7a6d5190e..3e87aa969 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -975,7 +975,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) {
@@ -985,7 +1006,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 5ee679da7..1251c9460 100644
--- a/src/core/mount.c
+++ b/src/core/mount.c
@@ -612,7 +612,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 fbb695d87..6be9ac84b 100644
--- a/src/core/path.c
+++ b/src/core/path.c
@@ -438,7 +438,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);
@@ -447,9 +452,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 1c3c6bb54..8b2bb29ed 100644
--- a/src/core/scope.c
+++ b/src/core/scope.c
@@ -171,7 +171,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 a89ff3f96..cc4ea1961 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -878,7 +878,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 0bebdbcbc..0285c49ae 100644
--- a/src/core/slice.c
+++ b/src/core/slice.c
@@ -150,7 +150,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 b70c3beb6..b1d844877 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 9606ac2b1..f67370b4c 100644
--- a/src/core/socket.c
+++ b/src/core/socket.c
@@ -1322,7 +1322,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 4dd6be8c9..bb1398f68 100644
--- a/src/core/swap.c
+++ b/src/core/swap.c
@@ -506,7 +506,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 8817ef21c..5f6440247 100644
--- a/src/core/target.c
+++ b/src/core/target.c
@@ -103,7 +103,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 940550194..79a754055 100644
--- a/src/core/timer.c
+++ b/src/core/timer.c
@@ -267,7 +267,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);
@@ -275,9 +280,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 b639d686a..ec4fa82ec 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -2856,27 +2856,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 ac5647a7f..11242c2a0 100644
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -301,8 +301,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);
 
@@ -538,7 +544,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);