Merge pull request #9366 from poettering/udev-wants-dep-fix

udev SYSTEMD_WANTS property fix

9 files changed, 79 insertions, 38 deletions

diff --git a/TODO b/TODO
index bcfe24ee3..ff1008acc 100644
--- a/TODO
+++ b/TODO
@@ -21,6 +21,10 @@ Janitorial Clean-ups:
 
 Features:
 
+* the stop-when-unneded feature should be reworked: there should be a queue of
+  units, and we should only enqeueu stop jobs from a defer event that processes
+  queue instead of right-away when we assume that a unit is now unneeded.
+
 * When reloading configuration PID 1 should reset all its properties to the
   original defaults before calling parse_config()
 
diff --git a/src/analyze/analyze-verify.c b/src/analyze/analyze-verify.c
index ae39c3149..ed369532d 100644
--- a/src/analyze/analyze-verify.c
+++ b/src/analyze/analyze-verify.c
@@ -108,10 +108,8 @@ static int verify_socket(Unit *u) {
 
         /* This makes sure instance is created if necessary. */
         r = socket_instantiate_service(SOCKET(u));
-        if (r < 0) {
-                log_unit_error_errno(u, r, "Socket cannot be started, failed to create instance: %m");
-                return r;
-        }
+        if (r < 0)
+                return log_unit_error_errno(u, r, "Socket cannot be started, failed to create instance: %m");
 
         /* This checks both type of sockets */
         if (UNIT_ISSET(SOCKET(u)->service)) {
@@ -227,19 +225,16 @@ static int verify_unit(Unit *u, bool check_man) {
 }
 
 int verify_units(char **filenames, UnitFileScope scope, bool check_man, bool run_generators) {
-        _cleanup_free_ char *var = NULL;
-        Manager *m = NULL;
-        FILE *serial = NULL;
-        FDSet *fdset = NULL;
-        char **filename;
-        int r = 0, k;
-
-        Unit *units[strv_length(filenames)];
-        int i, count = 0;
         const uint8_t flags = MANAGER_TEST_RUN_BASIC |
                               MANAGER_TEST_RUN_ENV_GENERATORS |
                               run_generators * MANAGER_TEST_RUN_GENERATORS;
 
+        _cleanup_(manager_freep) Manager *m = NULL;
+        Unit *units[strv_length(filenames)];
+        _cleanup_free_ char *var = NULL;
+        int r = 0, k, i, count = 0;
+        char **filename;
+
         if (strv_isempty(filenames))
                 return 0;
 
@@ -256,11 +251,9 @@ int verify_units(char **filenames, UnitFileScope scope, bool check_man, bool run
 
         log_debug("Starting manager...");
 
-        r = manager_startup(m, serial, fdset);
-        if (r < 0) {
-                log_error_errno(r, "Failed to start manager: %m");
-                goto finish;
-        }
+        r = manager_startup(m, NULL, NULL);
+        if (r < 0)
+                return log_error_errno(r, "Failed to start manager: %m");
 
         manager_clear_jobs(m);
 
@@ -292,8 +285,5 @@ int verify_units(char **filenames, UnitFileScope scope, bool check_man, bool run
                         r = k;
         }
 
-finish:
-        manager_free(m);
-
         return r;
 }
diff --git a/src/core/device.c b/src/core/device.c
index 4bffb9472..a2d00a0fb 100644
--- a/src/core/device.c
+++ b/src/core/device.c
@@ -6,6 +6,7 @@
 #include "libudev.h"
 
 #include "alloc-util.h"
+#include "bus-error.h"
 #include "dbus-device.h"
 #include "device.h"
 #include "log.h"
@@ -107,6 +108,7 @@ static void device_done(Unit *u) {
         assert(d);
 
         device_unset_sysfs(d);
+        d->wants_property = strv_free(d->wants_property);
 }
 
 static void device_set_state(Device *d, DeviceState state) {
@@ -276,6 +278,14 @@ static void device_dump(Unit *u, FILE *f, const char *prefix) {
                 prefix, device_state_to_string(d->state),
                 prefix, strna(d->sysfs),
                 prefix, strna(s));
+
+        if (!strv_isempty(d->wants_property)) {
+                char **i;
+
+                STRV_FOREACH(i, d->wants_property)
+                        fprintf(f, "%sudev SYSTEMD_WANTS: %s\n",
+                                prefix, *i);
+        }
 }
 
 _pure_ static UnitActiveState device_active_state(Unit *u) {
@@ -331,10 +341,12 @@ static int device_update_description(Unit *u, struct udev_device *dev, const cha
 }
 
 static int device_add_udev_wants(Unit *u, struct udev_device *dev) {
+        _cleanup_strv_free_ char **added = NULL;
         const char *wants, *property;
+        Device *d = DEVICE(u);
         int r;
 
-        assert(u);
+        assert(d);
         assert(dev);
 
         property = MANAGER_IS_USER(u->manager) ? "SYSTEMD_USER_WANTS" : "SYSTEMD_WANTS";
@@ -348,21 +360,21 @@ static int device_add_udev_wants(Unit *u, struct udev_device *dev) {
 
                 r = extract_first_word(&wants, &word, NULL, EXTRACT_QUOTES);
                 if (r == 0)
-                        return 0;
+                        break;
                 if (r == -ENOMEM)
                         return log_oom();
                 if (r < 0)
                         return log_unit_error_errno(u, r, "Failed to parse property %s with value %s: %m", property, wants);
 
-                if (unit_name_is_valid(word, UNIT_NAME_TEMPLATE) && DEVICE(u)->sysfs) {
+                if (unit_name_is_valid(word, UNIT_NAME_TEMPLATE) && d->sysfs) {
                         _cleanup_free_ char *escaped = NULL;
 
                         /* If the unit name is specified as template, then automatically fill in the sysfs path of the
                          * device as instance name, properly escaped. */
 
-                        r = unit_name_path_escape(DEVICE(u)->sysfs, &escaped);
+                        r = unit_name_path_escape(d->sysfs, &escaped);
                         if (r < 0)
-                                return log_unit_error_errno(u, r, "Failed to escape %s: %m", DEVICE(u)->sysfs);
+                                return log_unit_error_errno(u, r, "Failed to escape %s: %m", d->sysfs);
 
                         r = unit_name_replace_instance(word, escaped, &k);
                         if (r < 0)
@@ -378,7 +390,43 @@ static int device_add_udev_wants(Unit *u, struct udev_device *dev) {
                 r = unit_add_dependency_by_name(u, UNIT_WANTS, k, NULL, true, UNIT_DEPENDENCY_UDEV);
                 if (r < 0)
                         return log_unit_error_errno(u, r, "Failed to add Wants= dependency: %m");
+
+                r = strv_push(&added, k);
+                if (r < 0)
+                        return log_oom();
+
+                k = NULL;
         }
+
+        if (d->state != DEVICE_DEAD) {
+                char **i;
+
+                /* 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 appens 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;
+
+                        r = manager_add_job_by_name(u->manager, JOB_START, *i, JOB_FAIL, &error, NULL);
+                        if (r < 0)
+                                log_unit_warning_errno(u, r, "Failed to enqueue SYSTEMD_WANTS= job, ignoring: %s", bus_error_message(&error, r));
+                }
+        }
+
+        strv_free(d->wants_property);
+        d->wants_property = TAKE_PTR(added);
+
+        return 0;
 }
 
 static bool device_is_bound_by_mounts(Device *d, struct udev_device *dev) {
diff --git a/src/core/device.h b/src/core/device.h
index a5f9bbe58..a119b33e5 100644
--- a/src/core/device.h
+++ b/src/core/device.h
@@ -30,6 +30,9 @@ struct Device {
         DeviceFound found, deserialized_found, enumerated_found;
 
         bool bind_mounts;
+
+        /* The SYSTEMD_WANTS udev property for this device the last time we saw it */
+        char **wants_property;
 };
 
 extern const UnitVTable device_vtable;
diff --git a/src/core/manager.c b/src/core/manager.c
index df2f424f3..930df4e23 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -3108,8 +3108,9 @@ int manager_serialize(Manager *m, FILE *f, FDSet *fds, bool switching_root) {
         assert(m->n_reloading > 0);
         m->n_reloading--;
 
-        if (ferror(f))
-                return -EIO;
+        r = fflush_and_check(f);
+        if (r < 0)
+                return r;
 
         r = bus_fdset_add_all(m, fds);
         if (r < 0)
diff --git a/src/test/test-cgroup-mask.c b/src/test/test-cgroup-mask.c
index 06fd56773..d65959edf 100644
--- a/src/test/test-cgroup-mask.c
+++ b/src/test/test-cgroup-mask.c
@@ -17,8 +17,6 @@ static int test_cgroup_mask(void) {
         _cleanup_(rm_rf_physical_and_freep) char *runtime_dir = NULL;
         _cleanup_(manager_freep) Manager *m = NULL;
         Unit *son, *daughter, *parent, *root, *grandchild, *parent_deep;
-        FILE *serial = NULL;
-        FDSet *fdset = NULL;
         int r;
 
         r = enter_cgroup_subroot();
@@ -48,7 +46,7 @@ static int test_cgroup_mask(void) {
         m->default_tasks_max = (uint64_t) -1;
 
         assert_se(r >= 0);
-        assert_se(manager_startup(m, serial, fdset) >= 0);
+        assert_se(manager_startup(m, NULL, NULL) >= 0);
 
         /* Load units and verify hierarchy. */
         assert_se(manager_load_startable_unit_or_warn(m, "parent.slice", NULL, &parent) >= 0);
diff --git a/src/test/test-engine.c b/src/test/test-engine.c
index f0b95a33e..d072a15cb 100644
--- a/src/test/test-engine.c
+++ b/src/test/test-engine.c
@@ -15,8 +15,6 @@ int main(int argc, char *argv[]) {
         _cleanup_(sd_bus_error_free) sd_bus_error err = SD_BUS_ERROR_NULL;
         _cleanup_(manager_freep) Manager *m = NULL;
         Unit *a = NULL, *b = NULL, *c = NULL, *d = NULL, *e = NULL, *g = NULL, *h = NULL, *unit_with_multiple_dashes = NULL;
-        FILE *serial = NULL;
-        FDSet *fdset = NULL;
         Job *j;
         int r;
 
@@ -39,7 +37,7 @@ int main(int argc, char *argv[]) {
                 return EXIT_TEST_SKIP;
         }
         assert_se(r >= 0);
-        assert_se(manager_startup(m, serial, fdset) >= 0);
+        assert_se(manager_startup(m, NULL, NULL) >= 0);
 
         printf("Load1:\n");
         assert_se(manager_load_startable_unit_or_warn(m, "a.service", NULL, &a) >= 0);
diff --git a/src/test/test-sched-prio.c b/src/test/test-sched-prio.c
index 0ba28a931..c98628415 100644
--- a/src/test/test-sched-prio.c
+++ b/src/test/test-sched-prio.c
@@ -17,8 +17,6 @@ int main(int argc, char *argv[]) {
         _cleanup_(manager_freep) Manager *m = NULL;
         Unit *idle_ok, *idle_bad, *rr_ok, *rr_bad, *rr_sched;
         Service *ser;
-        FILE *serial = NULL;
-        FDSet *fdset = NULL;
         int r;
 
         r = enter_cgroup_subroot();
@@ -36,7 +34,7 @@ int main(int argc, char *argv[]) {
                 return EXIT_TEST_SKIP;
         }
         assert_se(r >= 0);
-        assert_se(manager_startup(m, serial, fdset) >= 0);
+        assert_se(manager_startup(m, NULL, NULL) >= 0);
 
         /* load idle ok */
         assert_se(manager_load_startable_unit_or_warn(m, "sched_idle_ok.service", NULL, &idle_ok) >= 0);
diff --git a/units/systemd-udev-trigger.service.in b/units/systemd-udev-trigger.service.in
index 3cdde9444..b60204ecc 100644
--- a/units/systemd-udev-trigger.service.in
+++ b/units/systemd-udev-trigger.service.in
@@ -19,4 +19,5 @@ ConditionPathIsReadWrite=/sys
 [Service]
 Type=oneshot
 RemainAfterExit=yes
-ExecStart=@rootbindir@/udevadm trigger --type=subsystems --action=add ; @rootbindir@/udevadm trigger --type=devices --action=add
+ExecStart=@rootbindir@/udevadm trigger --type=subsystems --action=add
+ExecStart=@rootbindir@/udevadm trigger --type=devices --action=add