Merge pull request #23848 from yuwata/core-device-systemd-wants

core: fix SYSTEMD_WANTS and StopWhenUnneeded=

src/core/device.c

@@ -547,18 +547,24 @@ static int device_add_udev_wants(Unit *u, sd_device *dev) {
         if (d->state != DEVICE_DEAD)
                 /* So here's a special hack, to compensate for the fact that the udev database's reload cycles are not
                  * synchronized with our own reload cycles: when we detect that the SYSTEMD_WANTS property of a device
-                 * changes while the device unit is already up, let's manually trigger any new units listed in it not
-                 * seen before. This typically happens during the boot-time switch root transition, as udev devices
-                 * will generally already be up in the initrd, but SYSTEMD_WANTS properties get then added through udev
-                 * rules only available on the host system, and thus only when the initial udev coldplug trigger runs.
+                 * changes while the device unit is already up, let's skip to trigger units that were already listed
+                 * and are active, and start units otherwise. This typically happens during the boot-time switch root
+                 * transition, as udev devices will generally already be up in the initrd, but SYSTEMD_WANTS properties
+                 * get then added through udev rules only available on the host system, and thus only when the initial
+                 * udev coldplug trigger runs.
                  *
                  * We do this only if the device has been up already when we parse this, as otherwise the usual
                  * dependency logic that is run from the dead → plugged transition will trigger these deps. */
                 STRV_FOREACH(i, added) {
                         _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
 
-                        if (strv_contains(d->wants_property, *i)) /* Was this unit already listed before? */
-                                continue;
+                        if (strv_contains(d->wants_property, *i)) {
+                                Unit *v;
+
+                                v = manager_get_unit(u->manager, *i);
+                                if (v && UNIT_IS_ACTIVE_OR_RELOADING(unit_active_state(v)))
+                                        continue; /* The unit was already listed and is running. */
+                        }
 
                         r = manager_add_job_by_name(u->manager, JOB_START, *i, JOB_FAIL, NULL, &error, NULL);
                         if (r < 0)

src/core/unit.c

@@ -5167,6 +5167,9 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) {
 
                                 unit_add_to_gc_queue(other);
 
+                                /* The unit 'other' may not be wanted by the unit 'u'. */
+                                unit_submit_to_stop_when_unneeded_queue(other);
+
                                 done = false;
                                 break;
                         }

test/units/assert.sh

@@ -4,10 +4,10 @@
 # utility functions for shell tests
 
 assert_true() {(
-    local rc
-
     set +ex
 
+    local rc
+
     "$@"
     rc=$?
     if [[ $rc -ne 0 ]]; then
@@ -47,10 +47,10 @@ assert_not_in() {(
 )}
 
 assert_rc() {(
-    local rc exp="${1?}"
-
     set +ex
 
+    local rc exp="${1?}"
+
     shift
     "$@"
     rc=$?

test/units/testsuite-17.07.sh

@@ -0,0 +1,205 @@
+#!/usr/bin/env bash
+# SPDX-License-Identifier: LGPL-2.1-or-later
+set -ex
+set -o pipefail
+
+# shellcheck source=test/units/assert.sh
+. "$(dirname "$0")"/assert.sh
+
+wait_service_active() {(
+    set +ex
+    for (( i = 0; i < 20; i++ )); do
+        if (( i != 0 )); then sleep 0.5; fi
+        if systemctl --quiet is-active "${1?}"; then
+            return 0
+        fi
+    done
+    return 1
+)}
+
+wait_service_inactive() {(
+    set +ex
+    for (( i = 0; i < 20; i++ )); do
+        if (( i != 0 )); then sleep 0.5; fi
+        systemctl --quiet is-active "${1?}"
+        if [[ "$?" == "3" ]]; then
+            return 0
+        fi
+    done
+    return 1
+)}
+
+mkdir -p /run/systemd/system
+cat >/run/systemd/system/both.service <<EOF
+[Service]
+ExecStart=sleep 1000
+EOF
+
+cat >/run/systemd/system/on-add.service <<EOF
+[Service]
+ExecStart=sleep 1000
+EOF
+
+cat >/run/systemd/system/on-change.service <<EOF
+[Service]
+ExecStart=sleep 1000
+EOF
+
+systemctl daemon-reload
+
+mkdir -p /run/udev/rules.d/
+cat >/run/udev/rules.d/50-testsuite.rules <<EOF
+SUBSYSTEM=="net", KERNEL=="dummy9?", OPTIONS="log_level=debug"
+SUBSYSTEM=="net", KERNEL=="dummy9?", ACTION=="add",    TAG+="systemd", ENV{SYSTEMD_WANTS}+="both.service", ENV{SYSTEMD_WANTS}+="on-add.service"
+SUBSYSTEM=="net", KERNEL=="dummy9?", ACTION=="change", TAG+="systemd", ENV{SYSTEMD_WANTS}+="both.service", ENV{SYSTEMD_WANTS}+="on-change.service"
+EOF
+
+udevadm control --reload
+
+# StopWhenUnneeded=no
+ip link add dummy99 type dummy
+udevadm wait --settle --timeout=30 /sys/class/net/dummy99
+wait_service_active both.service
+wait_service_active on-add.service
+assert_rc 3 systemctl --quiet is-active on-change.service
+systemctl stop both.service on-add.service
+
+udevadm trigger --action=change --settle /sys/class/net/dummy99
+udevadm info /sys/class/net/dummy99
+wait_service_active both.service
+assert_rc 3 systemctl --quiet is-active on-add.service
+wait_service_active on-change.service
+systemctl stop both.service on-change.service
+
+ip link del dummy99
+udevadm wait --settle --timeout=30 --removed /sys/class/net/dummy99
+assert_rc 3 systemctl --quiet is-active both.service
+assert_rc 3 systemctl --quiet is-active on-add.service
+assert_rc 3 systemctl --quiet is-active on-change.service
+
+# StopWhenUnneeded=yes
+cat >/run/systemd/system/both.service <<EOF
+[Unit]
+StopWhenUnneeded=yes
+
+[Service]
+ExecStart=sleep 1000
+Type=simple
+EOF
+
+cat >/run/systemd/system/on-add.service <<EOF
+[Unit]
+StopWhenUnneeded=yes
+
+[Service]
+ExecStart=sleep 1000
+Type=simple
+EOF
+
+cat >/run/systemd/system/on-change.service <<EOF
+[Unit]
+StopWhenUnneeded=yes
+
+[Service]
+ExecStart=echo changed
+RemainAfterExit=true
+Type=oneshot
+EOF
+
+systemctl daemon-reload
+
+# StopWhenUnneeded=yes (single device, only add event)
+ip link add dummy99 type dummy
+udevadm wait --settle --timeout=30 /sys/class/net/dummy99
+wait_service_active both.service
+wait_service_active on-add.service
+assert_rc 3 systemctl --quiet is-active on-change.service
+
+ip link del dummy99
+udevadm wait --settle --timeout=30 --removed /sys/class/net/dummy99
+wait_service_inactive both.service
+wait_service_inactive on-add.service
+assert_rc 3 systemctl --quiet is-active on-change.service
+
+# StopWhenUnneeded=yes (single device, add and change event)
+ip link add dummy99 type dummy
+udevadm wait --settle --timeout=30 /sys/class/net/dummy99
+wait_service_active both.service
+wait_service_active on-add.service
+assert_rc 3 systemctl --quiet is-active on-change.service
+
+udevadm trigger --action=change --settle /sys/class/net/dummy99
+assert_rc 0 systemctl --quiet is-active both.service
+wait_service_inactive on-add.service
+wait_service_active on-change.service
+
+ip link del dummy99
+udevadm wait --settle --timeout=30 --removed /sys/class/net/dummy99
+wait_service_inactive both.service
+assert_rc 3 systemctl --quiet is-active on-add.service
+wait_service_inactive on-change.service
+
+# StopWhenUnneeded=yes (multiple devices, only add events)
+ip link add dummy99 type dummy
+udevadm wait --settle --timeout=30 /sys/class/net/dummy99
+wait_service_active both.service
+wait_service_active on-add.service
+assert_rc 3 systemctl --quiet is-active on-change.service
+
+ip link add dummy98 type dummy
+udevadm wait --settle --timeout=30 /sys/class/net/dummy98
+assert_rc 0 systemctl --quiet is-active both.service
+assert_rc 0 systemctl --quiet is-active on-add.service
+assert_rc 3 systemctl --quiet is-active on-change.service
+
+ip link del dummy99
+udevadm wait --settle --timeout=30 --removed /sys/class/net/dummy99
+assert_rc 0 systemctl --quiet is-active both.service
+assert_rc 0 systemctl --quiet is-active on-add.service
+assert_rc 3 systemctl --quiet is-active on-change.service
+
+ip link del dummy98
+udevadm wait --settle --timeout=30 --removed /sys/class/net/dummy98
+wait_service_inactive both.service
+wait_service_inactive on-add.service
+assert_rc 3 systemctl --quiet is-active on-change.service
+
+# StopWhenUnneeded=yes (multiple devices, add and change events)
+ip link add dummy99 type dummy
+udevadm wait --settle --timeout=30 /sys/class/net/dummy99
+wait_service_active both.service
+wait_service_active on-add.service
+assert_rc 3 systemctl --quiet is-active on-change.service
+
+ip link add dummy98 type dummy
+udevadm wait --settle --timeout=30 /sys/class/net/dummy98
+assert_rc 0 systemctl --quiet is-active both.service
+assert_rc 0 systemctl --quiet is-active on-add.service
+assert_rc 3 systemctl --quiet is-active on-change.service
+
+udevadm trigger --action=change --settle /sys/class/net/dummy99
+assert_rc 0 systemctl --quiet is-active both.service
+assert_rc 0 systemctl --quiet is-active on-add.service
+wait_service_active on-change.service
+
+ip link del dummy98
+udevadm wait --settle --timeout=30 --removed /sys/class/net/dummy98
+assert_rc 0 systemctl --quiet is-active both.service
+wait_service_inactive on-add.service
+assert_rc 0 systemctl --quiet is-active on-change.service
+
+ip link del dummy99
+udevadm wait --settle --timeout=30 --removed /sys/class/net/dummy99
+wait_service_inactive both.service
+assert_rc 3 systemctl --quiet is-active on-add.service
+wait_service_inactive on-change.service
+
+# cleanup
+rm -f /run/udev/rules.d/50-testsuite.rules
+udevadm control --reload
+
+rm -f /run/systemd/system/on-add.service
+rm -f /run/systemd/system/on-change.service
+systemctl daemon-reload
+
+exit 0