From 4e8113bbbe10b96b9309d70d7616ba2879bad37f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 20 Jun 2018 19:36:06 +0200 Subject: [PATCH 1/7] update TODO --- TODO | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/TODO b/TODO index bcfe24ee30..ff1008accf 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() From 01d32c0030565664e91a9be8689ab99ce5e3b366 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 20 Jun 2018 19:38:30 +0200 Subject: [PATCH 2/7] core: use fflush_and_check() where appropriate --- src/core/manager.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index df2f424f33..930df4e23a 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) From 6133af603057b6f8c81d912fc773e3311805fafa Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 20 Jun 2018 21:39:28 +0200 Subject: [PATCH 3/7] units: let's use two ExecStart= lines instead of ; --- units/systemd-udev-trigger.service.in | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/units/systemd-udev-trigger.service.in b/units/systemd-udev-trigger.service.in index 3cdde94446..b60204eccc 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 From 76752516633e5a1011a91e9982e622086bd7fba9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 20 Jun 2018 22:19:26 +0200 Subject: [PATCH 4/7] tree-wide: pass NULL arguments to manager_startup() directly, avoid declaring unneeded variables --- src/analyze/analyze-verify.c | 4 +--- src/test/test-cgroup-mask.c | 4 +--- src/test/test-engine.c | 4 +--- src/test/test-sched-prio.c | 4 +--- 4 files changed, 4 insertions(+), 12 deletions(-) diff --git a/src/analyze/analyze-verify.c b/src/analyze/analyze-verify.c index ae39c3149f..1f33e560ac 100644 --- a/src/analyze/analyze-verify.c +++ b/src/analyze/analyze-verify.c @@ -229,8 +229,6 @@ 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; @@ -256,7 +254,7 @@ int verify_units(char **filenames, UnitFileScope scope, bool check_man, bool run log_debug("Starting manager..."); - r = manager_startup(m, serial, fdset); + r = manager_startup(m, NULL, NULL); if (r < 0) { log_error_errno(r, "Failed to start manager: %m"); goto finish; diff --git a/src/test/test-cgroup-mask.c b/src/test/test-cgroup-mask.c index 06fd567738..d65959edf1 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 f0b95a33e9..d072a15cb1 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 0ba28a931b..c986284155 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); From 4d2a7ffd5ba47da26dde13a537fd0d3adc6e126e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 20 Jun 2018 22:22:03 +0200 Subject: [PATCH 5/7] analyze: use log_unit_error_errno()'s return value where we can --- src/analyze/analyze-verify.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/analyze/analyze-verify.c b/src/analyze/analyze-verify.c index 1f33e560ac..7f606ee24b 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)) { From b5c9144cb78cfe4ca8cd672a42e164f7fcbf4249 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 20 Jun 2018 22:22:22 +0200 Subject: [PATCH 6/7] analyze: use _cleanup_ for freeing the manager object --- src/analyze/analyze-verify.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/analyze/analyze-verify.c b/src/analyze/analyze-verify.c index 7f606ee24b..ed369532d4 100644 --- a/src/analyze/analyze-verify.c +++ b/src/analyze/analyze-verify.c @@ -225,17 +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; - 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; @@ -253,10 +252,8 @@ int verify_units(char **filenames, UnitFileScope scope, bool check_man, bool run log_debug("Starting manager..."); r = manager_startup(m, NULL, NULL); - if (r < 0) { - log_error_errno(r, "Failed to start manager: %m"); - goto finish; - } + if (r < 0) + return log_error_errno(r, "Failed to start manager: %m"); manager_clear_jobs(m); @@ -288,8 +285,5 @@ int verify_units(char **filenames, UnitFileScope scope, bool check_man, bool run r = k; } -finish: - manager_free(m); - return r; } From 88116909ec60724ddce47feb2cc40c52bdb81bdf Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 20 Jun 2018 22:54:55 +0200 Subject: [PATCH 7/7] core: explicitly trigger changing udev SYSTEMD_WANTS properties This compensates for the unsynchronized reload cycles of systemd and udev: we manually trigger the deps listed in SYSTEMD_WANTS properties if they change for device units that are already up. That way all deps defined that way will be triggered at least once: the first time the unit goes up by the usual dependency logic, and if it already is up by the device.c specific logic. Fixes: #9323 --- src/core/device.c | 58 +++++++++++++++++++++++++++++++++++++++++++---- src/core/device.h | 3 +++ 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 4bffb94726..a2d00a0fbe 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 a5f9bbe58c..a119b33e57 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;