Merge pull request #27746 from yuwata/unit-bidirectional-dep

core/unit: make unit dependency always bidirectional

man/systemd.unit.xml

@@ -856,16 +856,18 @@
         <term><varname>JoinsNamespaceOf=</varname></term>
 
         <listitem><para>For units that start processes (such as service units), lists one or more other units
-        whose network and/or temporary file namespace to join. This only applies to unit types which support
-        the <varname>PrivateNetwork=</varname>, <varname>NetworkNamespacePath=</varname>,
+        whose network and/or temporary file namespace to join. If this is specified on a unit (say, a.service
+        has <varname>JoinsNamespaceOf=b.service</varname>), then this the inverse dependency
+        (<varname>JoinsNamespaceOf=a.service</varname> for b.service) is implied. This only applies to unit
+        types which support the <varname>PrivateNetwork=</varname>, <varname>NetworkNamespacePath=</varname>,
         <varname>PrivateIPC=</varname>, <varname>IPCNamespacePath=</varname>, and
         <varname>PrivateTmp=</varname> directives (see
         <citerefentry><refentrytitle>systemd.exec</refentrytitle><manvolnum>5</manvolnum></citerefentry> for
         details). If a unit that has this setting set is started, its processes will see the same
         <filename>/tmp/</filename>, <filename>/var/tmp/</filename>, IPC namespace and network namespace as
-        one listed unit that is started. If multiple listed units are already started, it is not defined
-        which namespace is joined. Note that this setting only has an effect if
-        <varname>PrivateNetwork=</varname>/<varname>NetworkNamespacePath=</varname>,
+        one listed unit that is started. If multiple listed units are already started and these do not share
+        their namespace, then it is not defined which namespace is joined. Note that this setting only has an
+        effect if <varname>PrivateNetwork=</varname>/<varname>NetworkNamespacePath=</varname>,
         <varname>PrivateIPC=</varname>/<varname>IPCNamespacePath=</varname> and/or
         <varname>PrivateTmp=</varname> is enabled for both the unit that joins the namespace and the unit
         whose namespace is joined.</para></listitem>

src/core/unit.c

@@ -1053,50 +1053,9 @@ static int unit_per_dependency_type_hashmap_update(
         if (r < 0)
                 return r;
 
-
         return 1;
 }
 
-static int unit_add_dependency_hashmap(
-                Hashmap **dependencies,
-                UnitDependency d,
-                Unit *other,
-                UnitDependencyMask origin_mask,
-                UnitDependencyMask destination_mask) {
-
-        Hashmap *per_type;
-        int r;
-
-        assert(dependencies);
-        assert(other);
-        assert(origin_mask < _UNIT_DEPENDENCY_MASK_FULL);
-        assert(destination_mask < _UNIT_DEPENDENCY_MASK_FULL);
-        assert(origin_mask > 0 || destination_mask > 0);
-
-        /* Ensure the top-level dependency hashmap exists that maps UnitDependency → Hashmap(Unit* →
-         * UnitDependencyInfo) */
-        r = hashmap_ensure_allocated(dependencies, NULL);
-        if (r < 0)
-                return r;
-
-        /* Acquire the inner hashmap, that maps Unit* → UnitDependencyInfo, for the specified dependency
-         * type, and if it's missing allocate it and insert it. */
-        per_type = hashmap_get(*dependencies, UNIT_DEPENDENCY_TO_PTR(d));
-        if (!per_type) {
-                per_type = hashmap_new(NULL);
-                if (!per_type)
-                        return -ENOMEM;
-
-                r = hashmap_put(*dependencies, UNIT_DEPENDENCY_TO_PTR(d), per_type);
-                if (r < 0) {
-                        hashmap_free(per_type);
-                        return r;
-                }
-        }
-
-        return unit_per_dependency_type_hashmap_update(per_type, other, origin_mask, destination_mask);
-}
-
 static void unit_merge_dependencies(Unit *u, Unit *other) {
         Hashmap *deps;
         void *dt; /* Actually of type UnitDependency, except that we don't bother casting it here,
@@ -3104,11 +3063,38 @@ bool unit_job_is_applicable(Unit *u, JobType j) {
         }
 }
 
-int unit_add_dependency(
+static Hashmap *unit_get_dependency_hashmap_per_type(Unit *u, UnitDependency d) {
+        Hashmap *deps;
+
+        assert(u);
+        assert(d >= 0 && d < _UNIT_DEPENDENCY_MAX);
+
+        deps = hashmap_get(u->dependencies, UNIT_DEPENDENCY_TO_PTR(d));
+        if (!deps) {
+                _cleanup_hashmap_free_ Hashmap *h = NULL;
+
+                h = hashmap_new(NULL);
+                if (!h)
+                        return NULL;
+
+                if (hashmap_ensure_put(&u->dependencies, NULL, UNIT_DEPENDENCY_TO_PTR(d), h) < 0)
+                        return NULL;
+
+                deps = TAKE_PTR(h);
+        }
+
+        return deps;
+}
+
+typedef enum NotifyDependencyFlags {
+        NOTIFY_DEPENDENCY_UPDATE_FROM = 1 << 0,
+        NOTIFY_DEPENDENCY_UPDATE_TO   = 1 << 1,
+} NotifyDependencyFlags;
+
+static int unit_add_dependency_impl(
                 Unit *u,
                 UnitDependency d,
                 Unit *other,
-                bool add_reference,
                 UnitDependencyMask mask) {
 
         static const UnitDependency inverse_table[_UNIT_DEPENDENCY_MAX] = {
@@ -3144,12 +3130,78 @@ int unit_add_dependency(
                 [UNIT_IN_SLICE]               = UNIT_SLICE_OF,
                 [UNIT_SLICE_OF]               = UNIT_IN_SLICE,
         };
+
+        Hashmap *u_deps, *other_deps;
+        UnitDependencyInfo u_info, u_info_old, other_info, other_info_old;
+        NotifyDependencyFlags flags = 0;
+        int r;
+
+        assert(u);
+        assert(other);
+        assert(d >= 0 && d < _UNIT_DEPENDENCY_MAX);
+        assert(inverse_table[d] >= 0 && inverse_table[d] < _UNIT_DEPENDENCY_MAX);
+        assert(mask > 0 && mask < _UNIT_DEPENDENCY_MASK_FULL);
+
+        /* Ensure the following two hashmaps for each unit exist:
+         * - the top-level dependency hashmap that maps UnitDependency → Hashmap(Unit* → UnitDependencyInfo),
+         * - the inner hashmap, that maps Unit* → UnitDependencyInfo, for the specified dependency type. */
+        u_deps = unit_get_dependency_hashmap_per_type(u, d);
+        if (!u_deps)
+                return -ENOMEM;
+
+        other_deps = unit_get_dependency_hashmap_per_type(other, inverse_table[d]);
+        if (!other_deps)
+                return -ENOMEM;
+
+        /* Save the original dependency info. */
+        u_info.data = u_info_old.data = hashmap_get(u_deps, other);
+        other_info.data = other_info_old.data = hashmap_get(other_deps, u);
+
+        /* Update dependency info. */
+        u_info.origin_mask |= mask;
+        other_info.destination_mask |= mask;
+
+        /* Save updated dependency info. */
+        if (u_info.data != u_info_old.data) {
+                r = hashmap_replace(u_deps, other, u_info.data);
+                if (r < 0)
+                        return r;
+
+                flags = NOTIFY_DEPENDENCY_UPDATE_FROM;
+        }
+
+        if (other_info.data != other_info_old.data) {
+                r = hashmap_replace(other_deps, u, other_info.data);
+                if (r < 0) {
+                        if (u_info.data != u_info_old.data) {
+                                /* Restore the old dependency. */
+                                if (u_info_old.data)
+                                        (void) hashmap_update(u_deps, other, u_info_old.data);
+                                else
+                                        hashmap_remove(u_deps, other);
+                        }
+                        return r;
+                }
+
+                flags |= NOTIFY_DEPENDENCY_UPDATE_TO;
+        }
+
+        return flags;
+}
+
+int unit_add_dependency(
+                Unit *u,
+                UnitDependency d,
+                Unit *other,
+                bool add_reference,
+                UnitDependencyMask mask) {
+
         UnitDependencyAtom a;
         int r;
 
         /* Helper to know whether sending a notification is necessary or not: if the dependency is already
          * there, no need to notify! */
-        bool notify, notify_other = false;
+        NotifyDependencyFlags notify_flags;
 
         assert(u);
         assert(d >= 0 && d < _UNIT_DEPENDENCY_MAX);
@@ -3205,36 +3257,24 @@ int unit_add_dependency(
                 return log_unit_error_errno(u, SYNTHETIC_ERRNO(EINVAL),
                                             "Requested dependency SliceOf=%s refused (%s is not a cgroup unit).", other->id, other->id);
 
-        r = unit_add_dependency_hashmap(&u->dependencies, d, other, mask, 0);
+        r = unit_add_dependency_impl(u, d, other, mask);
         if (r < 0)
                 return r;
-        notify = r > 0;
-
-        if (inverse_table[d] != _UNIT_DEPENDENCY_INVALID && inverse_table[d] != d) {
-                r = unit_add_dependency_hashmap(&other->dependencies, inverse_table[d], u, 0, mask);
-                if (r < 0)
-                        return r;
-                notify_other = r > 0;
-        }
+        notify_flags = r;
 
         if (add_reference) {
-                r = unit_add_dependency_hashmap(&u->dependencies, UNIT_REFERENCES, other, mask, 0);
+                r = unit_add_dependency_impl(u, UNIT_REFERENCES, other, mask);
                 if (r < 0)
                         return r;
-                notify = notify || r > 0;
-
-                r = unit_add_dependency_hashmap(&other->dependencies, UNIT_REFERENCED_BY, u, 0, mask);
-                if (r < 0)
-                        return r;
-                notify_other = notify_other || r > 0;
+                notify_flags |= r;
         }
 
-        if (notify)
+        if (FLAGS_SET(notify_flags, NOTIFY_DEPENDENCY_UPDATE_FROM))
                 unit_add_to_dbus_queue(u);
-        if (notify_other)
+        if (FLAGS_SET(notify_flags, NOTIFY_DEPENDENCY_UPDATE_TO))
                 unit_add_to_dbus_queue(other);
 
-        return notify || notify_other;
+        return notify_flags != 0;
 }
 
 int unit_add_two_dependencies(Unit *u, UnitDependency d, UnitDependency e, Unit *other, bool add_reference, UnitDependencyMask mask) {
@@ -4890,6 +4930,7 @@ int unit_require_mounts_for(Unit *u, const char *path, UnitDependencyMask mask)
 int unit_setup_exec_runtime(Unit *u) {
         _cleanup_(exec_shared_runtime_unrefp) ExecSharedRuntime *esr = NULL;
         _cleanup_(dynamic_creds_unrefp) DynamicCreds *dcreds = NULL;
+        _cleanup_set_free_ Set *units = NULL;
         ExecRuntime **rt;
         ExecContext *ec;
         size_t offset;
@@ -4907,8 +4948,12 @@ int unit_setup_exec_runtime(Unit *u) {
         ec = unit_get_exec_context(u);
         assert(ec);
 
+        r = unit_get_transitive_dependency_set(u, UNIT_ATOM_JOINS_NAMESPACE_OF, &units);
+        if (r < 0)
+                return r;
+
         /* Try to get it from somebody else */
-        UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_JOINS_NAMESPACE_OF) {
+        SET_FOREACH(other, units) {
                 r = exec_shared_runtime_acquire(u->manager, NULL, other->id, false, &esr);
                 if (r < 0)
                         return r;
@@ -6113,6 +6158,33 @@ int unit_get_dependency_array(const Unit *u, UnitDependencyAtom atom, Unit ***re
         return (int) n;
 }
 
+int unit_get_transitive_dependency_set(Unit *u, UnitDependencyAtom atom, Set **ret) {
+        _cleanup_set_free_ Set *units = NULL, *queue = NULL;
+        Unit *other;
+        int r;
+
+        assert(u);
+        assert(ret);
+
+        /* Similar to unit_get_dependency_array(), but also search the same dependency in other units. */
+
+        do {
+                UNIT_FOREACH_DEPENDENCY(other, u, atom) {
+                        r = set_ensure_put(&units, NULL, other);
+                        if (r < 0)
+                                return r;
+                        if (r == 0)
+                                continue;
+                        r = set_ensure_put(&queue, NULL, other);
+                        if (r < 0)
+                                return r;
+                }
+        } while ((u = set_steal_first(queue)));
+
+        *ret = TAKE_PTR(units);
+        return 0;
+}
+
 const ActivationDetailsVTable * const activation_details_vtable[_UNIT_TYPE_MAX] = {
         [UNIT_PATH] = &activation_details_path_vtable,
         [UNIT_TIMER] = &activation_details_timer_vtable,

src/core/unit.h

@@ -810,6 +810,7 @@ static inline const UnitVTable* UNIT_VTABLE(const Unit *u) {
 
 Unit* unit_has_dependency(const Unit *u, UnitDependencyAtom atom, Unit *other);
 int unit_get_dependency_array(const Unit *u, UnitDependencyAtom atom, Unit ***ret_array);
+int unit_get_transitive_dependency_set(Unit *u, UnitDependencyAtom atom, Set **ret);
 
 static inline Hashmap* unit_get_dependencies(Unit *u, UnitDependency d) {
         return hashmap_get(u->dependencies, UNIT_DEPENDENCY_TO_PTR(d));

test/testsuite-23.units/testsuite-23-joins-namespace-of-1.service

@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Service]
+Type=notify
+NotifyAccess=all
+MountAPIVFS=yes
+PrivateTmp=yes
+ExecStart=/bin/bash -c 'touch /tmp/shared-private-file && systemd-notify --ready && sleep infinity'

test/testsuite-23.units/testsuite-23-joins-namespace-of-2.service

@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Unit]
+JoinsNamespaceOf=testsuite-23-joins-namespace-of-1.service
+
+[Service]
+Type=oneshot
+MountAPIVFS=yes
+PrivateTmp=yes
+ExecStart=test -e /tmp/shared-private-file
+ExecStart=touch /tmp/hoge

test/testsuite-23.units/testsuite-23-joins-namespace-of-3.service

@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Unit]
+JoinsNamespaceOf=testsuite-23-joins-namespace-of-1.service
+
+[Service]
+Type=oneshot
+MountAPIVFS=yes
+PrivateTmp=yes
+ExecStart=test -e /tmp/shared-private-file
+ExecStart=test -e /tmp/hoge

test/testsuite-23.units/testsuite-23-joins-namespace-of-4.service

@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Unit]
+JoinsNamespaceOf=testsuite-23-joins-namespace-of-5.service
+
+[Service]
+Type=notify
+NotifyAccess=all
+MountAPIVFS=yes
+PrivateTmp=yes
+ExecStart=/bin/bash -c 'touch /tmp/shared-private-file && systemd-notify --ready && sleep infinity'

test/testsuite-23.units/testsuite-23-joins-namespace-of-5.service

@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Service]
+Type=oneshot
+MountAPIVFS=yes
+PrivateTmp=yes
+ExecStart=test -e /tmp/shared-private-file

test/testsuite-23.units/testsuite-23-joins-namespace-of-6.service

@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Unit]
+JoinsNamespaceOf=testsuite-23-joins-namespace-of-8.service
+
+[Service]
+Type=notify
+NotifyAccess=all
+MountAPIVFS=yes
+PrivateTmp=yes
+ExecStart=/bin/bash -c 'touch /tmp/shared-private-file-x && systemd-notify --ready && sleep infinity'

test/testsuite-23.units/testsuite-23-joins-namespace-of-7.service

@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Unit]
+JoinsNamespaceOf=testsuite-23-joins-namespace-of-8.service
+
+[Service]
+Type=oneshot
+MountAPIVFS=yes
+PrivateTmp=yes
+ExecStart=test -e /tmp/shared-private-file-x
+ExecStart=test ! -e /tmp/shared-private-file-y
+ExecStart=touch /tmp/hoge

test/testsuite-23.units/testsuite-23-joins-namespace-of-8.service

@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Service]
+Type=notify
+NotifyAccess=all
+MountAPIVFS=yes
+PrivateTmp=yes
+ExecStartPre=test -e /tmp/shared-private-file-x
+ExecStartPre=test -e /tmp/hoge
+ExecStart=/bin/bash -c 'touch /tmp/shared-private-file-y && systemd-notify --ready && sleep infinity'

test/testsuite-23.units/testsuite-23-joins-namespace-of-9.service

@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Unit]
+JoinsNamespaceOf=testsuite-23-joins-namespace-of-8.service
+
+[Service]
+Type=oneshot
+MountAPIVFS=yes
+PrivateTmp=yes
+ExecStart=test -e /tmp/shared-private-file-x
+ExecStart=test -e /tmp/shared-private-file-y
+ExecStart=test -e /tmp/hoge

test/units/testsuite-23.JoinsNamespaceOf.sh

@@ -0,0 +1,31 @@
+#!/usr/bin/env bash
+# SPDX-License-Identifier: LGPL-2.1-or-later
+
+set -eux
+set -o pipefail
+
+# Test JoinsNamespaceOf= with PrivateTmp=yes
+
+systemd-analyze log-level debug
+systemd-analyze log-target journal
+
+# simple case
+systemctl start testsuite-23-joins-namespace-of-1.service
+systemctl start testsuite-23-joins-namespace-of-2.service
+systemctl start testsuite-23-joins-namespace-of-3.service
+systemctl stop testsuite-23-joins-namespace-of-1.service
+
+# inverse dependency
+systemctl start testsuite-23-joins-namespace-of-4.service
+systemctl start testsuite-23-joins-namespace-of-5.service
+systemctl stop testsuite-23-joins-namespace-of-4.service
+
+# transitive dependency
+systemctl start testsuite-23-joins-namespace-of-6.service
+systemctl start testsuite-23-joins-namespace-of-7.service
+systemctl start testsuite-23-joins-namespace-of-8.service
+systemctl start testsuite-23-joins-namespace-of-9.service
+systemctl stop testsuite-23-joins-namespace-of-6.service
+systemctl stop testsuite-23-joins-namespace-of-8.service
+
+systemd-analyze log-level info