core: delay adding target dependencies until all units are loaded and aliases resolve...

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.

diff --git a/src/core/manager.c b/src/core/manager.c
index 9a71f5f2e7e83e72eb6a846c70b35b4c4880ffc8..e67f7446c723975c845de9f6f8fa4ab0c6bc2920 100644 (file)
--- 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 80304a4010ce615d9ab4046a9499f9c6796e6fe3..28c5da225b16e624e6596ad38f96a90134425f25 100644 (file)
--- 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 90ec73231b92349bc92e9078dfb6cd05ac308da1..72f475ab140b3c1228bd83b95ca3dcc86134313e 100644 (file)
--- 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 e9370a4b93c94c96e10754ed7a60008e656e1aa2..9f50d81cc88ac81c0e633533a2f282f754421a94 100644 (file)
--- 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);