From 19496554e23ea4861ce780430052dcf86a2ffcba Mon Sep 17 00:00:00 2001 From: Michal Sekletar Date: Fri, 23 Mar 2018 15:28:06 +0100 Subject: [PATCH] core: delay adding target dependencies until all units are loaded and aliases resolved (#8381) Currently we add target dependencies while we are loading units. This can create ordering loops even if configuration doesn't contain any loop. Take for example following configuration, $ systemctl get-default multi-user.target $ cat /etc/systemd/system/test.service [Unit] After=default.target [Service] ExecStart=/bin/true [Install] WantedBy=multi-user.target If we encounter such unit file early during manager start-up (e.g. load queue is dispatched while enumerating devices due to SYSTEMD_WANTS in udev rules) we would add stub unit default.target and we order it Before test.service. At the same time we add implicit Before to multi-user.target. Later we merge two units and we create ordering cycle in the process. To fix the issue we will now never add any target dependencies until we loaded all the unit files and resolved all the aliases. --- src/core/manager.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/core/manager.h | 3 +++ src/core/unit.c | 49 ++++++++++++++++--------------------------------- src/core/unit.h | 5 +++++ 4 files changed, 65 insertions(+), 33 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 9a71f5f..e67f744 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1670,6 +1670,42 @@ Unit *manager_get_unit(Manager *m, const char *name) { return hashmap_get(m->units, name); } +static int manager_dispatch_target_deps_queue(Manager *m) { + Unit *u; + unsigned k; + int r = 0; + + static const UnitDependency deps[] = { + UNIT_REQUIRED_BY, + UNIT_REQUISITE_OF, + UNIT_WANTED_BY, + UNIT_BOUND_BY + }; + + assert(m); + + while ((u = m->target_deps_queue)) { + assert(u->in_target_deps_queue); + + LIST_REMOVE(target_deps_queue, u->manager->target_deps_queue, u); + u->in_target_deps_queue = false; + + for (k = 0; k < ELEMENTSOF(deps); k++) { + Unit *target; + Iterator i; + void *v; + + HASHMAP_FOREACH_KEY(v, target, u->dependencies[deps[k]], i) { + r = unit_add_default_target_dependency(u, target); + if (r < 0) + return r; + } + } + } + + return r; +} + unsigned manager_dispatch_load_queue(Manager *m) { Unit *u; unsigned n = 0; @@ -1693,6 +1729,11 @@ unsigned manager_dispatch_load_queue(Manager *m) { } m->dispatching_load_queue = false; + + /* Dispatch the units waiting for their target dependencies to be added now, as all targets that we know about + * should be loaded and have aliases resolved */ + (void) manager_dispatch_target_deps_queue(m); + return n; } diff --git a/src/core/manager.h b/src/core/manager.h index 80304a4..28c5da2 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -144,6 +144,9 @@ struct Manager { /* Units whose cgroup ran empty */ LIST_HEAD(Unit, cgroup_empty_queue); + /* Target units whose default target dependencies haven't been set yet */ + LIST_HEAD(Unit, target_deps_queue); + sd_event *event; /* This maps PIDs we care about to units that are interested in. We allow multiple units to he interested in diff --git a/src/core/unit.c b/src/core/unit.c index 90ec732..72f475a 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -649,6 +649,9 @@ void unit_free(Unit *u) { if (u->in_cleanup_queue) LIST_REMOVE(cleanup_queue, u->manager->cleanup_queue, u); + if (u->in_target_deps_queue) + LIST_REMOVE(target_deps_queue, u->manager->target_deps_queue, u); + safe_close(u->ip_accounting_ingress_map_fd); safe_close(u->ip_accounting_egress_map_fd); @@ -1338,6 +1341,18 @@ int unit_load_fragment_and_dropin_optional(Unit *u) { return unit_load_dropin(unit_follow_merge(u)); } +void unit_add_to_target_deps_queue(Unit *u) { + Manager *m = u->manager; + + assert(u); + + if (u->in_target_deps_queue) + return; + + LIST_PREPEND(target_deps_queue, m->target_deps_queue, u); + u->in_target_deps_queue = true; +} + int unit_add_default_target_dependency(Unit *u, Unit *target) { assert(u); assert(target); @@ -1364,35 +1379,6 @@ int unit_add_default_target_dependency(Unit *u, Unit *target) { return unit_add_dependency(target, UNIT_AFTER, u, true, UNIT_DEPENDENCY_DEFAULT); } -static int unit_add_target_dependencies(Unit *u) { - - static const UnitDependency deps[] = { - UNIT_REQUIRED_BY, - UNIT_REQUISITE_OF, - UNIT_WANTED_BY, - UNIT_BOUND_BY - }; - - unsigned k; - int r = 0; - - assert(u); - - for (k = 0; k < ELEMENTSOF(deps); k++) { - Unit *target; - Iterator i; - void *v; - - HASHMAP_FOREACH_KEY(v, target, u->dependencies[deps[k]], i) { - r = unit_add_default_target_dependency(u, target); - if (r < 0) - return r; - } - } - - return r; -} - static int unit_add_slice_dependencies(Unit *u) { UnitDependencyMask mask; assert(u); @@ -1521,10 +1507,7 @@ int unit_load(Unit *u) { } if (u->load_state == UNIT_LOADED) { - - r = unit_add_target_dependencies(u); - if (r < 0) - goto fail; + unit_add_to_target_deps_queue(u); r = unit_add_slice_dependencies(u); if (r < 0) diff --git a/src/core/unit.h b/src/core/unit.h index e9370a4..9f50d81 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -231,6 +231,9 @@ struct Unit { /* cgroup empty queue */ LIST_FIELDS(Unit, cgroup_empty_queue); + /* Target dependencies queue */ + LIST_FIELDS(Unit, target_deps_queue); + /* PIDs we keep an eye on. Note that a unit might have many * more, but these are the ones we care enough about to * process SIGCHLD for */ @@ -336,6 +339,7 @@ struct Unit { bool in_gc_queue:1; bool in_cgroup_realize_queue:1; bool in_cgroup_empty_queue:1; + bool in_target_deps_queue:1; bool sent_dbus_new_signal:1; @@ -632,6 +636,7 @@ void unit_add_to_load_queue(Unit *u); void unit_add_to_dbus_queue(Unit *u); void unit_add_to_cleanup_queue(Unit *u); void unit_add_to_gc_queue(Unit *u); +void unit_add_to_target_deps_queue(Unit *u); int unit_merge(Unit *u, Unit *other); int unit_merge_by_name(Unit *u, const char *other); -- 2.7.4