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