core: warn when merged units have conflicting dependencies

A unit should not Conflict with itself. It also does not make
much sense for a unit to be After or Before itself, or to
trigger itself in some way.

If one of those dependency types is encountered, warn, instead
of dropping it silently like other dependency types.

% build/systemd-analyze verify test/loopy3.service
...
Dependency Conflicts dropped when merging unit loopy4.service into loopy3.service
Dependency ConflictedBy dropped when merging unit loopy4.service into loopy3.service

diff --git a/src/core/unit.c b/src/core/unit.c
index 092673256d550dc42d9c25028651adcd9ca2880a..a5f6b2ead071fdf3a86ee310dbabebb955597f7f 100644 (file)
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -69,6 +69,8 @@ const UnitVTable * const unit_vtable[_UNIT_TYPE_MAX] = {
         [UNIT_SCOPE] = &scope_vtable
 };
 
+static int maybe_warn_about_dependency(const char *id, const char *other, UnitDependency dependency);
+
 Unit *unit_new(Manager *m, size_t size) {
         Unit *u;
 
@@ -583,7 +585,7 @@ static void merge_names(Unit *u, Unit *other) {
                 assert_se(hashmap_replace(u->manager->units, t, u) == 0);
 }
 
-static void merge_dependencies(Unit *u, Unit *other, UnitDependency d) {
+static void merge_dependencies(Unit *u, Unit *other, const char *other_id, UnitDependency d) {
         Iterator i;
         Unit *back;
         int r;
@@ -599,7 +601,8 @@ static void merge_dependencies(Unit *u, Unit *other, UnitDependency d) {
                 for (k = 0; k < _UNIT_DEPENDENCY_MAX; k++) {
                         /* Do not add dependencies between u and itself */
                         if (back == u) {
-                                set_remove(back->dependencies[k], other);
+                                if (set_remove(back->dependencies[k], other))
+                                        maybe_warn_about_dependency(u->id, other_id, k);
                         } else {
                                 r = set_remove_and_put(back->dependencies[k], other, u);
                                 if (r == -EEXIST)
@@ -611,7 +614,9 @@ static void merge_dependencies(Unit *u, Unit *other, UnitDependency d) {
         }
 
         /* Also do not move dependencies on u to itself */
-        set_remove(other->dependencies[d], u);
+        back = set_remove(other->dependencies[d], u);
+        if (back)
+                maybe_warn_about_dependency(u->id, other_id, d);
 
         complete_move(&u->dependencies[d], &other->dependencies[d]);
 
@@ -621,6 +626,7 @@ static void merge_dependencies(Unit *u, Unit *other, UnitDependency d) {
 
 int unit_merge(Unit *u, Unit *other) {
         UnitDependency d;
+        const char *other_id = NULL;
 
         assert(u);
         assert(other);
@@ -651,6 +657,9 @@ int unit_merge(Unit *u, Unit *other) {
         if (!UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(other)))
                 return -EEXIST;
 
+        if (other->id)
+                other_id = strdupa(other->id);
+
         /* Merge names */
         merge_names(u, other);
 
@@ -660,7 +669,7 @@ int unit_merge(Unit *u, Unit *other) {
 
         /* Merge dependencies */
         for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++)
-                merge_dependencies(u, other, d);
+                merge_dependencies(u, other, other_id, d);
 
         other->load_state = UNIT_MERGED;
         other->merged_into = u;
@@ -1039,6 +1048,9 @@ static int unit_add_slice_dependencies(Unit *u) {
         if (UNIT_ISSET(u->slice))
                 return unit_add_two_dependencies(u, UNIT_AFTER, UNIT_WANTS, UNIT_DEREF(u->slice), true);
 
+        if (streq(u->id, SPECIAL_ROOT_SLICE))
+                return 0;
+
         return unit_add_two_dependencies_by_name(u, UNIT_AFTER, UNIT_WANTS, SPECIAL_ROOT_SLICE, NULL, true);
 }
 
@@ -1945,6 +1957,50 @@ bool unit_job_is_applicable(Unit *u, JobType j) {
         }
 }
 
+static int maybe_warn_about_dependency(const char *id, const char *other, UnitDependency dependency) {
+        switch (dependency) {
+        case UNIT_REQUIRES:
+        case UNIT_REQUIRES_OVERRIDABLE:
+        case UNIT_WANTS:
+        case UNIT_REQUISITE:
+        case UNIT_REQUISITE_OVERRIDABLE:
+        case UNIT_BINDS_TO:
+        case UNIT_PART_OF:
+        case UNIT_REQUIRED_BY:
+        case UNIT_REQUIRED_BY_OVERRIDABLE:
+        case UNIT_WANTED_BY:
+        case UNIT_BOUND_BY:
+        case UNIT_CONSISTS_OF:
+        case UNIT_REFERENCES:
+        case UNIT_REFERENCED_BY:
+        case UNIT_PROPAGATES_RELOAD_TO:
+        case UNIT_RELOAD_PROPAGATED_FROM:
+        case UNIT_JOINS_NAMESPACE_OF:
+                return 0;
+
+        case UNIT_CONFLICTS:
+        case UNIT_CONFLICTED_BY:
+        case UNIT_BEFORE:
+        case UNIT_AFTER:
+        case UNIT_ON_FAILURE:
+        case UNIT_TRIGGERS:
+        case UNIT_TRIGGERED_BY:
+                if (streq_ptr(id, other))
+                        log_warning_unit(id, "Dependency %s=%s dropped from unit %s",
+                                         unit_dependency_to_string(dependency), id, other);
+                else
+                        log_warning_unit(id, "Dependency %s=%s dropped from unit %s merged into %s",
+                                         unit_dependency_to_string(dependency), id,
+                                         strna(other), id);
+                return -EINVAL;
+
+        case _UNIT_DEPENDENCY_MAX:
+        case _UNIT_DEPENDENCY_INVALID:
+                break;
+        }
+        assert_not_reached("Invalid dependency type");
+}
+
 int unit_add_dependency(Unit *u, UnitDependency d, Unit *other, bool add_reference) {
 
         static const UnitDependency inverse_table[_UNIT_DEPENDENCY_MAX] = {
@@ -1974,6 +2030,7 @@ int unit_add_dependency(Unit *u, UnitDependency d, Unit *other, bool add_referen
                 [UNIT_JOINS_NAMESPACE_OF] = UNIT_JOINS_NAMESPACE_OF,
         };
         int r, q = 0, v = 0, w = 0;
+        Unit *orig_u = u, *orig_other = other;
 
         assert(u);
         assert(d >= 0 && d < _UNIT_DEPENDENCY_MAX);
@@ -1984,8 +2041,10 @@ int unit_add_dependency(Unit *u, UnitDependency d, Unit *other, bool add_referen
 
         /* We won't allow dependencies on ourselves. We will not
          * consider them an error however. */
-        if (u == other)
+        if (u == other) {
+                maybe_warn_about_dependency(orig_u->id, orig_other->id, d);
                 return 0;
+        }
 
         r = set_ensure_allocated(&u->dependencies[d], trivial_hash_func, trivial_compare_func);
         if (r < 0)

diff --git a/test/loopy3.service b/test/loopy3.service
new file mode 100644 (file)
index 0000000..606e26b
--- /dev/null
+++ b/test/loopy3.service
@@ -0,0 +1,5 @@
+[Service]
+ExecStart=/bin/true
+
+[Unit]
+Conflicts=loopy4.service

diff --git a/test/loopy4.service b/test/loopy4.service
new file mode 120000 (symlink)
index 0000000..43e5658
--- /dev/null
+++ b/test/loopy4.service
@@ -0,0 +1 @@
+loopy3.service
\ No newline at end of file