From f997080b4d17a40b59b398e4354b6368d9c85f69 Mon Sep 17 00:00:00 2001 From: Ivan Shapovalov 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);