| 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 9f6bd84b2..e4c79415d 100644 |
| |
| |
| @@ -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 1d77292f9..43d7607a3 100644 |
| |
| |
| @@ -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 1cc103c29..4ff882721 100644 |
| |
| |
| @@ -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 bc9b7ec62..203a6a0a1 100644 |
| |
| |
| @@ -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 c971330af..3ae0eb462 100644 |
| |
| |
| @@ -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 e5ea79fec..51e36fa8b 100644 |
| |
| |
| @@ -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 b41db7872..ae6614fbf 100644 |
| |
| |
| @@ -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 15e29be14..7781b4e62 100644 |
| |
| |
| @@ -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 ae9819d01..61ff9d331 100644 |
| |
| |
| @@ -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 b70c3beb6..b1d844877 100644 |
| |
| |
| @@ -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 88aae4815..760de0203 100644 |
| |
| |
| @@ -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 5c19af5d9..369abf0f5 100644 |
| |
| |
| @@ -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 33fb66bc3..2411a8e75 100644 |
| |
| |
| @@ -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 45744c7de..48cf9c16a 100644 |
| |
| |
| @@ -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 a6558ee23..565455bd6 100644 |
| |
| |
| @@ -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 291bc77a7..7ebc489c8 100644 |
| |
| |
| @@ -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); |
| |