From ccd419f0e7c267c82b89287d967509615ef929f4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Jun 2018 18:03:02 +0200 Subject: [PATCH 01/22] core: modernize device_set_sysfs() a bit --- src/core/device.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index b3f8eb4e02..725aa9d3bf 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -55,8 +55,8 @@ static void device_unset_sysfs(Device *d) { } static int device_set_sysfs(Device *d, const char *sysfs) { + _cleanup_free_ char *copy = NULL; Device *first; - char *copy; int r; assert(d); @@ -80,12 +80,10 @@ static int device_set_sysfs(Device *d, const char *sysfs) { r = hashmap_replace(UNIT(d)->manager->devices_by_sysfs, copy, first); if (r < 0) { LIST_REMOVE(same_sysfs, first, d); - free(copy); return r; } - d->sysfs = copy; - + d->sysfs = TAKE_PTR(copy); return 0; } From 6e0f878ee26f4622778fd31b18e9cdf9e0038e0c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Jun 2018 18:03:30 +0200 Subject: [PATCH 02/22] core: use FLAGS_SET() macro at one more place --- src/core/device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/device.c b/src/core/device.c index 725aa9d3bf..30707d07a7 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -167,7 +167,7 @@ static int device_found_to_string_many(DeviceFound flags, char **ret) { assert(ret); for (i = 0; device_found_map[i].name; i++) { - if ((flags & device_found_map[i].flag) != device_found_map[i].flag) + if (!FLAGS_SET(flags, device_found_map[i].flag)) continue; if (!strextend_with_separator(&s, ",", device_found_map[i].name, NULL)) From 43ba7d71e550908be101c6d00eb3e8f0e372db4e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Jun 2018 18:03:57 +0200 Subject: [PATCH 03/22] core: use device_found_to_string_many() result only on success --- src/core/device.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 30707d07a7..9d09a219d5 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -222,8 +222,8 @@ static int device_serialize(Unit *u, FILE *f, FDSet *fds) { unit_serialize_item(u, f, "state", device_state_to_string(d->state)); - (void) device_found_to_string_many(d->found, &s); - unit_serialize_item(u, f, "found", s); + if (device_found_to_string_many(d->found, &s) >= 0) + unit_serialize_item(u, f, "found", s); return 0; } From ea8ec43bb5c6cfa9794cf6f292006b3aec82fc8c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Jun 2018 18:04:33 +0200 Subject: [PATCH 04/22] core: modernize device_update_description() a bit --- src/core/device.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 9d09a219d5..ba69fdc70d 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -324,19 +324,18 @@ static int device_update_description(Unit *u, struct udev_device *dev, const cha _cleanup_free_ char *j; j = strjoin(model, " ", label); - if (j) - r = unit_set_description(u, j); - else - r = -ENOMEM; + if (!j) + return log_oom(); + + r = unit_set_description(u, j); } else r = unit_set_description(u, model); } else r = unit_set_description(u, path); - if (r < 0) - log_unit_error_errno(u, r, "Failed to set device description: %m"); + return log_unit_error_errno(u, r, "Failed to set device description: %m"); - return r; + return 0; } static int device_add_udev_wants(Unit *u, struct udev_device *dev) { From 86cdffd1069f5ebb6ded3988e5a2eca59b0a6e81 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Jun 2018 18:05:09 +0200 Subject: [PATCH 05/22] core: log when unit_add_dependency() fails for some reason Also, proceed, as there's little we can do in this case. --- src/core/device.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index ba69fdc70d..b4f162d1b7 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -409,7 +409,7 @@ static bool device_is_bound_by_mounts(Device *d, struct udev_device *dev) { return d->bind_mounts; } -static int device_upgrade_mount_deps(Unit *u) { +static void device_upgrade_mount_deps(Unit *u) { Unit *other; Iterator i; void *v; @@ -423,9 +423,8 @@ static int device_upgrade_mount_deps(Unit *u) { r = unit_add_dependency(other, UNIT_BINDS_TO, u, true, UNIT_DEPENDENCY_UDEV); if (r < 0) - return r; + log_unit_warning_errno(u, r, "Failed to add BindsTo= dependency between device and mount unit, ignoring: %m"); } - return 0; } static int device_setup_unit(Manager *m, struct udev_device *dev, const char *path, bool main) { From 9d4c195c6482b16528c5a25b85d42607bedffe3b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Jun 2018 20:02:59 +0200 Subject: [PATCH 06/22] core: split out bus initialization from manager_setup() --- src/core/manager.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 7fc31ce569..108110fee9 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1463,6 +1463,25 @@ static bool manager_dbus_is_running(Manager *m, bool deserialized) { return true; } +static void manager_setup_bus(Manager *m) { + assert(m); + + /* Let's set up our private bus connection now, unconditionally */ + (void) bus_init_private(m); + + /* If we are in --user mode also connect to the system bus now */ + if (MANAGER_IS_USER(m)) + (void) bus_init_system(m); + + /* Let's connect to the bus now, but only if the unit is supposed to be up */ + if (manager_dbus_is_running(m, MANAGER_IS_RELOADING(m))) { + (void) bus_init_api(m); + + if (MANAGER_IS_SYSTEM(m)) + (void) bus_init_system(m); + } +} + int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { int r; @@ -1543,20 +1562,8 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { /* This shouldn't fail, except if things are really broken. */ return r; - /* Let's set up our private bus connection now, unconditionally */ - (void) bus_init_private(m); - - /* If we are in --user mode also connect to the system bus now */ - if (MANAGER_IS_USER(m)) - (void) bus_init_system(m); - - /* Let's connect to the bus now, but only if the unit is supposed to be up */ - if (manager_dbus_is_running(m, !!serialization)) { - (void) bus_init_api(m); - - if (MANAGER_IS_SYSTEM(m)) - (void) bus_init_system(m); - } + /* Connect to the bus if we are good for it */ + manager_setup_bus(m); /* Now that we are connected to all possible busses, let's deserialize who is tracking us. */ (void) bus_track_coldplug(m, &m->subscribed, false, m->deserialized_subscribed); From 8bb2c8c94d7d6333a80f7fb214708e6daab09c93 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Jun 2018 20:29:04 +0200 Subject: [PATCH 07/22] core: improve error logging a bit --- src/core/device.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index b4f162d1b7..7773f18fd5 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -254,14 +254,14 @@ static int device_deserialize_item(Unit *u, const char *key, const char *value, state = device_state_from_string(value); if (state < 0) - log_unit_debug(u, "Failed to parse state value: %s", value); + log_unit_debug(u, "Failed to parse state value, ignoring: %s", value); else d->deserialized_state = state; } else if (streq(key, "found")) { r = device_found_from_string_many(value, &d->found); if (r < 0) - log_unit_debug(u, "Failed to parse found value: %s", value); + log_unit_debug(u, "Failed to parse found value, ignoring: %s", value); } else log_unit_debug(u, "Unknown serialization key: %s", key); @@ -570,7 +570,7 @@ static int device_process_new(Manager *m, struct udev_device *dev) { r = extract_first_word(&alias, &word, NULL, EXTRACT_QUOTES); if (r == 0) - return 0; + break; if (r == -ENOMEM) return log_oom(); if (r < 0) @@ -581,6 +581,8 @@ static int device_process_new(Manager *m, struct udev_device *dev) { else log_warning("SYSTEMD_ALIAS for %s is not an absolute path, ignoring: %s", sysfs, word); } + + return 0; } static void device_update_found_one(Device *d, bool add, DeviceFound found, bool now) { @@ -749,7 +751,7 @@ static void device_enumerate(Manager *m) { if (!m->udev_monitor) { m->udev_monitor = udev_monitor_new_from_netlink(m->udev, "udev"); if (!m->udev_monitor) { - log_oom(); + log_error_errno(errno, "Failed to allocate udev monitor: %m"); goto fail; } @@ -781,7 +783,7 @@ static void device_enumerate(Manager *m) { e = udev_enumerate_new(m->udev); if (!e) { - log_oom(); + log_error_errno(errno, "Failed to alloacte udev enumerator: %m"); goto fail; } @@ -812,7 +814,13 @@ static void device_enumerate(Manager *m) { dev = udev_device_new_from_syspath(m->udev, sysfs); if (!dev) { - log_oom(); + if (errno == ENOMEM) { + log_oom(); + goto fail; + } + + /* If we can't create a device, don't bother, it probably just disappeared. */ + log_debug_errno(errno, "Failed to create udev device object for %s: %m", sysfs); continue; } @@ -888,7 +896,7 @@ static int device_dispatch_io(sd_event_source *source, int fd, uint32_t revents, if (streq(action, "remove")) { r = swap_process_device_remove(m, dev); if (r < 0) - log_error_errno(r, "Failed to process swap device remove event: %m"); + log_warning_errno(r, "Failed to process swap device remove event, ignoring: %m"); /* If we get notified that a device was removed by * udev, then it's completely gone, hence unset all @@ -901,7 +909,7 @@ static int device_dispatch_io(sd_event_source *source, int fd, uint32_t revents, r = swap_process_device_new(m, dev); if (r < 0) - log_error_errno(r, "Failed to process swap device new event: %m"); + log_warning_errno(r, "Failed to process swap device new event, ignoring: %m"); manager_dispatch_load_queue(m); @@ -986,6 +994,8 @@ int device_found_node(Manager *m, const char *node, bool add, DeviceFound found, } bool device_shall_be_bound_by(Unit *device, Unit *u) { + assert(device); + assert(u); if (u->type != UNIT_MOUNT) return false; From 87934b36b13b48af6d412c3473d5c3eb11e6a32c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Jun 2018 21:16:50 +0200 Subject: [PATCH 08/22] core: split out reload propagation into its own function --- src/core/device.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 7773f18fd5..e7c537f63a 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -838,6 +838,24 @@ fail: device_shutdown(m); } +static void device_propagate_reload_by_sysfs(Manager *m, const char *sysfs) { + Device *d, *l, *n; + int r; + + assert(m); + assert(sysfs); + + l = hashmap_get(m->devices_by_sysfs, sysfs); + LIST_FOREACH_SAFE(same_sysfs, d, n, l) { + if (d->state == DEVICE_DEAD) + continue; + + r = manager_propagate_reload(m, UNIT(d), JOB_REPLACE, NULL); + if (r < 0) + log_warning_errno(r, "Failed to propagate reload, ignoring: %m"); + } +} + static int device_dispatch_io(sd_event_source *source, int fd, uint32_t revents, void *userdata) { _cleanup_(udev_device_unrefp) struct udev_device *dev = NULL; Manager *m = userdata; @@ -875,20 +893,8 @@ static int device_dispatch_io(sd_event_source *source, int fd, uint32_t revents, return 0; } - if (streq(action, "change")) { - Unit *u; - Device *d, *l, *n; - - l = hashmap_get(m->devices_by_sysfs, sysfs); - LIST_FOREACH_SAFE(same_sysfs, d, n, l) { - u = &d->meta; - if (u && UNIT_VTABLE(u)->active_state(u) == UNIT_ACTIVE) { - r = manager_propagate_reload(m, u, JOB_REPLACE, NULL); - if (r < 0) - log_error_errno(r, "Failed to propagate reload: %m"); - } - } - } + if (streq(action, "change")) + device_propagate_reload_by_sysfs(m, sysfs); /* A change event can signal that a device is becoming ready, in particular if * the device is using the SYSTEMD_READY logic in udev From 485ae697ba48bcd3c38a01d0af17831ac87a5c55 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Jun 2018 18:06:05 +0200 Subject: [PATCH 09/22] core: rework device_found_node() prototype let's drop the "now" argument, it's exactly what MANAGER_IS_RUNNING() returns, hence let's use that instead to simplify things. Moreover, let's change the add/found argument pair to become found/mask, which allows us to change multiple flags at the same time into opposing directions, which will be useful later on. Also, let's change the return type to void. It's a notifier call where callers will ignore the return value anyway as it is nothing actionable. Should not change behaviour. --- src/core/device.c | 56 +++++++++++++++++++++++------------------------ src/core/device.h | 2 +- src/core/mount.c | 4 ++-- src/core/swap.c | 4 ++-- 4 files changed, 33 insertions(+), 33 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index e7c537f63a..26a5014e28 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -585,19 +585,19 @@ static int device_process_new(Manager *m, struct udev_device *dev) { return 0; } -static void device_update_found_one(Device *d, bool add, DeviceFound found, bool now) { +static void device_update_found_one(Device *d, DeviceFound found, DeviceFound mask) { DeviceFound n, previous; assert(d); - n = add ? (d->found | found) : (d->found & ~found); + n = (d->found & ~mask) | (found & mask); if (n == d->found) return; previous = d->found; d->found = n; - if (!now) + if (MANAGER_IS_RUNNING(UNIT(d)->manager)) return; /* Didn't exist before, but does now? if so, generate a new invocation ID for it */ @@ -622,26 +622,23 @@ static void device_update_found_one(Device *d, bool add, DeviceFound found, bool device_set_state(d, DEVICE_DEAD); device_unset_sysfs(d); } - } -static int device_update_found_by_sysfs(Manager *m, const char *sysfs, bool add, DeviceFound found, bool now) { +static void device_update_found_by_sysfs(Manager *m, const char *sysfs, DeviceFound found, DeviceFound mask) { Device *d, *l, *n; assert(m); assert(sysfs); - if (found == DEVICE_NOT_FOUND) - return 0; + if (mask == 0) + return; l = hashmap_get(m->devices_by_sysfs, sysfs); LIST_FOREACH_SAFE(same_sysfs, d, n, l) - device_update_found_one(d, add, found, now); - - return 0; + device_update_found_one(d, found, mask); } -static int device_update_found_by_name(Manager *m, const char *path, bool add, DeviceFound found, bool now) { +static int device_update_found_by_name(Manager *m, const char *path, DeviceFound found, DeviceFound mask) { _cleanup_free_ char *e = NULL; Unit *u; int r; @@ -649,7 +646,7 @@ static int device_update_found_by_name(Manager *m, const char *path, bool add, D assert(m); assert(path); - if (found == DEVICE_NOT_FOUND) + if (mask == 0) return 0; r = unit_name_from_path(path, ".device", &e); @@ -660,7 +657,7 @@ static int device_update_found_by_name(Manager *m, const char *path, bool add, D if (!u) return 0; - device_update_found_one(DEVICE(u), add, found, now); + device_update_found_one(DEVICE(u), found, mask); return 0; } @@ -828,8 +825,7 @@ static void device_enumerate(Manager *m) { continue; (void) device_process_new(m, dev); - - device_update_found_by_sysfs(m, sysfs, true, DEVICE_FOUND_UDEV_DB, false); + device_update_found_by_sysfs(m, sysfs, DEVICE_FOUND_UDEV_DB, DEVICE_FOUND_UDEV_DB); } return; @@ -907,7 +903,7 @@ static int device_dispatch_io(sd_event_source *source, int fd, uint32_t revents, /* If we get notified that a device was removed by * udev, then it's completely gone, hence unset all * found bits */ - device_update_found_by_sysfs(m, sysfs, false, DEVICE_FOUND_UDEV|DEVICE_FOUND_MOUNT|DEVICE_FOUND_SWAP, true); + device_update_found_by_sysfs(m, sysfs, 0, DEVICE_FOUND_UDEV|DEVICE_FOUND_MOUNT|DEVICE_FOUND_SWAP); } else if (device_is_ready(dev)) { @@ -920,14 +916,14 @@ static int device_dispatch_io(sd_event_source *source, int fd, uint32_t revents, manager_dispatch_load_queue(m); /* The device is found now, set the udev found bit */ - device_update_found_by_sysfs(m, sysfs, true, DEVICE_FOUND_UDEV, true); + device_update_found_by_sysfs(m, sysfs, DEVICE_FOUND_UDEV, DEVICE_FOUND_UDEV); } else { /* The device is nominally around, but not ready for * us. Hence unset the udev bit, but leave the rest * around. */ - device_update_found_by_sysfs(m, sysfs, false, DEVICE_FOUND_UDEV, true); + device_update_found_by_sysfs(m, sysfs, 0, DEVICE_FOUND_UDEV); } return 0; @@ -945,7 +941,7 @@ static bool device_supported(void) { return read_only <= 0; } -int device_found_node(Manager *m, const char *node, bool add, DeviceFound found, bool now) { +void device_found_node(Manager *m, const char *node, DeviceFound found, DeviceFound mask) { _cleanup_(udev_device_unrefp) struct udev_device *dev = NULL; struct stat st; @@ -953,7 +949,7 @@ int device_found_node(Manager *m, const char *node, bool add, DeviceFound found, assert(node); if (!device_supported()) - return 0; + return; /* This is called whenever we find a device referenced in * /proc/swaps or /proc/self/mounts. Such a device might be @@ -962,9 +958,9 @@ int device_found_node(Manager *m, const char *node, bool add, DeviceFound found, * yet. In this case we will set the device unit to * "tentative" state. */ - if (add) { + if ((found & mask) != 0) { if (!path_startswith(node, "/dev")) - return 0; + return; /* We make an extra check here, if the device node * actually exists. If it's missing, then this is an @@ -978,14 +974,18 @@ int device_found_node(Manager *m, const char *node, bool add, DeviceFound found, if (stat(node, &st) >= 0) { if (!S_ISBLK(st.st_mode) && !S_ISCHR(st.st_mode)) - return 0; + return; dev = udev_device_new_from_devnum(m->udev, S_ISBLK(st.st_mode) ? 'b' : 'c', st.st_rdev); - if (!dev && errno != ENOENT) - return log_error_errno(errno, "Failed to get udev device from devnum %u:%u: %m", major(st.st_rdev), minor(st.st_rdev)); + if (!dev && errno != ENOENT) { + log_error_errno(errno, "Failed to get udev device from devnum %u:%u: %m", major(st.st_rdev), minor(st.st_rdev)); + return; + } - } else if (errno != ENOENT) - return log_error_errno(errno, "Failed to stat device node file %s: %m", node); + } else if (errno != ENOENT) { + log_error_errno(errno, "Failed to stat device node file %s: %m", node); + return; + } /* If the device is known in the kernel and newly * appeared, then we'll create a device unit for it, @@ -996,7 +996,7 @@ int device_found_node(Manager *m, const char *node, bool add, DeviceFound found, } /* Update the device unit's state, should it exist */ - return device_update_found_by_name(m, node, add, found, now); + (void) device_update_found_by_name(m, node, found, mask); } bool device_shall_be_bound_by(Unit *device, Unit *u) { diff --git a/src/core/device.h b/src/core/device.h index f188640c59..ab534ca24e 100644 --- a/src/core/device.h +++ b/src/core/device.h @@ -37,7 +37,7 @@ struct Device { extern const UnitVTable device_vtable; -int device_found_node(Manager *m, const char *node, bool add, DeviceFound found, bool now); +void device_found_node(Manager *m, const char *node, DeviceFound found, DeviceFound mask); bool device_shall_be_bound_by(Unit *device, Unit *u); DEFINE_CAST(DEVICE, Device); diff --git a/src/core/mount.c b/src/core/mount.c index 3e20f78021..64115dc21b 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1634,7 +1634,7 @@ static int mount_load_proc_self_mountinfo(Manager *m, bool set_flags) { if (cunescape(path, UNESCAPE_RELAX, &p) < 0) return log_oom(); - (void) device_found_node(m, d, true, DEVICE_FOUND_MOUNT, set_flags); + device_found_node(m, d, DEVICE_FOUND_MOUNT, DEVICE_FOUND_MOUNT); k = mount_setup_unit(m, d, p, options, fstype, set_flags); if (r == 0 && k < 0) @@ -1896,7 +1896,7 @@ static int mount_dispatch_io(sd_event_source *source, int fd, uint32_t revents, continue; /* Let the device units know that the device is no longer mounted */ - (void) device_found_node(m, what, false, DEVICE_FOUND_MOUNT, true); + device_found_node(m, what, 0, DEVICE_FOUND_MOUNT); } return 0; diff --git a/src/core/swap.c b/src/core/swap.c index 6cd703c61d..693561dd90 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -1112,7 +1112,7 @@ static int swap_load_proc_swaps(Manager *m, bool set_flags) { if (cunescape(dev, UNESCAPE_RELAX, &d) < 0) return log_oom(); - device_found_node(m, d, true, DEVICE_FOUND_SWAP, set_flags); + device_found_node(m, d, DEVICE_FOUND_SWAP, DEVICE_FOUND_SWAP); k = swap_process_new(m, d, prio, set_flags); if (k < 0) @@ -1167,7 +1167,7 @@ static int swap_dispatch_io(sd_event_source *source, int fd, uint32_t revents, v } if (swap->what) - device_found_node(m, swap->what, false, DEVICE_FOUND_SWAP, true); + device_found_node(m, swap->what, 0, DEVICE_FOUND_SWAP); } else if (swap->just_activated) { From 34c20ee09fc0a83a8d08ee28518ac2b77dfe3373 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Jun 2018 21:35:30 +0200 Subject: [PATCH 10/22] core: split out device validation from device_found_node() Let's separate the validate step out. Also, let's update some comments which have long ceased to be true. No change in behaviour. --- src/core/device.c | 96 ++++++++++++++++++++++++++++------------------- 1 file changed, 58 insertions(+), 38 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 26a5014e28..e296ffa418 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -941,56 +941,76 @@ static bool device_supported(void) { return read_only <= 0; } -void device_found_node(Manager *m, const char *node, DeviceFound found, DeviceFound mask) { - _cleanup_(udev_device_unrefp) struct udev_device *dev = NULL; +static int validate_node(Manager *m, const char *node, struct udev_device **ret) { struct stat st; + assert(m); + assert(node); + assert(ret); + + /* Validates a device node that showed up in /proc/swaps or /proc/self/mountinfo if it makes sense for us to + * track. Note that this validator is fine within missing device nodes, but not with badly set up ones! */ + + if (!path_startswith(node, "/dev")) { + *ret = NULL; + return 0; /* bad! */ + } + + if (stat(node, &st) < 0) { + if (errno != ENOENT) + return log_error_errno(errno, "Failed to stat() device node file %s: %m", node); + + *ret = NULL; + return 1; /* good! (though missing) */ + + } else { + _cleanup_(udev_device_unrefp) struct udev_device *dev = NULL; + + if (!S_ISBLK(st.st_mode) && !S_ISCHR(st.st_mode)) { + *ret = NULL; + return 0; /* bad! */ + } + + dev = udev_device_new_from_devnum(m->udev, S_ISBLK(st.st_mode) ? 'b' : 'c', st.st_rdev); + if (!dev) { + if (errno != ENOENT) + return log_error_errno(errno, "Failed to get udev device from devnum %u:%u: %m", major(st.st_rdev), minor(st.st_rdev)); + + *ret = NULL; + return 1; /* good! (though missing) */ + } + + *ret = TAKE_PTR(dev); + return 1; /* good! */ + } +} + +void device_found_node(Manager *m, const char *node, DeviceFound found, DeviceFound mask) { + int r; + assert(m); assert(node); if (!device_supported()) return; - /* This is called whenever we find a device referenced in - * /proc/swaps or /proc/self/mounts. Such a device might be - * mounted/enabled at a time where udev has not finished - * probing it yet, and we thus haven't learned about it - * yet. In this case we will set the device unit to - * "tentative" state. */ + if (mask == 0) + return; + + /* This is called whenever we find a device referenced in /proc/swaps or /proc/self/mounts. Such a device might + * be mounted/enabled at a time where udev has not finished probing it yet, and we thus haven't learned about + * it yet. In this case we will set the device unit to "tentative" state. */ if ((found & mask) != 0) { - if (!path_startswith(node, "/dev")) - return; + _cleanup_(udev_device_unrefp) struct udev_device *dev = NULL; - /* We make an extra check here, if the device node - * actually exists. If it's missing, then this is an - * indication that device was unplugged but is still - * referenced in /proc/swaps or - * /proc/self/mountinfo. Note that this check doesn't - * really cover all cases where a device might be gone - * away, since drives that can have a medium inserted - * will still have a device node even when the medium - * is not there... */ + /* If the device is known in the kernel and newly appeared, then we'll create a device unit for it, + * under the name referenced in /proc/swaps or /proc/self/mountinfo. But first, let's validate if + * everything is alright with the device node. */ - if (stat(node, &st) >= 0) { - if (!S_ISBLK(st.st_mode) && !S_ISCHR(st.st_mode)) - return; - - dev = udev_device_new_from_devnum(m->udev, S_ISBLK(st.st_mode) ? 'b' : 'c', st.st_rdev); - if (!dev && errno != ENOENT) { - log_error_errno(errno, "Failed to get udev device from devnum %u:%u: %m", major(st.st_rdev), minor(st.st_rdev)); - return; - } - - } else if (errno != ENOENT) { - log_error_errno(errno, "Failed to stat device node file %s: %m", node); - return; - } - - /* If the device is known in the kernel and newly - * appeared, then we'll create a device unit for it, - * under the name referenced in /proc/swaps or - * /proc/self/mountinfo. */ + r = validate_node(m, node, &dev); + if (r <= 0) + return; /* Don't create a device unit for this if the device node is borked. */ (void) device_setup_unit(m, dev, node, false); } From e5ca27b772267bd18109eb460fb6653a920e551e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Jun 2018 22:52:02 +0200 Subject: [PATCH 11/22] udev: add helper udev_device_new_from_stat_rdev() This is a simple wrapper around udev_device_new_from_devnum(), and uses the data from a struct stat's .st_rdev field to derive the udev_device object. --- src/core/device.c | 19 ++++++++----------- src/core/swap.c | 15 +++++++++++---- src/journal/journalctl.c | 13 ++++++------- src/shared/udev-util.c | 23 +++++++++++++++++++++++ src/shared/udev-util.h | 2 ++ 5 files changed, 50 insertions(+), 22 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index e296ffa418..e92b982439 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -943,6 +943,7 @@ static bool device_supported(void) { static int validate_node(Manager *m, const char *node, struct udev_device **ret) { struct stat st; + int r; assert(m); assert(node); @@ -966,19 +967,15 @@ static int validate_node(Manager *m, const char *node, struct udev_device **ret) } else { _cleanup_(udev_device_unrefp) struct udev_device *dev = NULL; - if (!S_ISBLK(st.st_mode) && !S_ISCHR(st.st_mode)) { - *ret = NULL; - return 0; /* bad! */ - } - - dev = udev_device_new_from_devnum(m->udev, S_ISBLK(st.st_mode) ? 'b' : 'c', st.st_rdev); - if (!dev) { - if (errno != ENOENT) - return log_error_errno(errno, "Failed to get udev device from devnum %u:%u: %m", major(st.st_rdev), minor(st.st_rdev)); - + r = udev_device_new_from_stat_rdev(m->udev, &st, &dev); + if (r == -ENOENT) { *ret = NULL; return 1; /* good! (though missing) */ - } + } else if (r == -ENOTTY) { + *ret = NULL; + return 0; /* bad! (not a device node but some other kind of file system node) */ + } else if (r < 0) + return log_error_errno(r, "Failed to get udev device from devnum %u:%u: %m", major(st.st_rdev), minor(st.st_rdev)); *ret = TAKE_PTR(dev); return 1; /* good! */ diff --git a/src/core/swap.c b/src/core/swap.c index 693561dd90..ab9057f29d 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -255,15 +255,19 @@ static int swap_load_devnode(Swap *s) { _cleanup_(udev_device_unrefp) struct udev_device *d = NULL; struct stat st; const char *p; + int r; assert(s); if (stat(s->what, &st) < 0 || !S_ISBLK(st.st_mode)) return 0; - d = udev_device_new_from_devnum(UNIT(s)->manager->udev, 'b', st.st_rdev); - if (!d) + r = udev_device_new_from_stat_rdev(UNIT(s)->manager->udev, &st, &d); + if (r < 0) { + log_unit_full(UNIT(s), r == -ENOENT ? LOG_DEBUG : LOG_WARNING, r, + "Failed to allocate udev device for swap %s: %m", s->what); return 0; + } p = udev_device_get_devnode(d); if (!p) @@ -443,9 +447,12 @@ static int swap_process_new(Manager *m, const char *device, int prio, bool set_f if (stat(device, &st) < 0 || !S_ISBLK(st.st_mode)) return 0; - d = udev_device_new_from_devnum(m->udev, 'b', st.st_rdev); - if (!d) + r = udev_device_new_from_stat_rdev(m->udev, &st, &d); + if (r < 0) { + log_full_errno(r == -ENOENT ? LOG_DEBUG : LOG_WARNING, r, + "Failed to allocate udev device for swap %s: %m", device); return 0; + } /* Add the main device node */ dn = udev_device_get_devnode(d); diff --git a/src/journal/journalctl.c b/src/journal/journalctl.c index 61d29986ce..9ad1b31fa3 100644 --- a/src/journal/journalctl.c +++ b/src/journal/journalctl.c @@ -182,11 +182,11 @@ typedef struct BootId { } BootId; static int add_matches_for_device(sd_journal *j, const char *devpath) { - int r; _cleanup_(udev_unrefp) struct udev *udev = NULL; _cleanup_(udev_device_unrefp) struct udev_device *device = NULL; struct udev_device *d = NULL; struct stat st; + int r; assert(j); assert(devpath); @@ -200,13 +200,12 @@ static int add_matches_for_device(sd_journal *j, const char *devpath) { if (!udev) return log_oom(); - r = stat(devpath, &st); - if (r < 0) - log_error_errno(errno, "Couldn't stat file: %m"); + if (stat(devpath, &st) < 0) + return log_error_errno(errno, "Couldn't stat file: %m"); - d = device = udev_device_new_from_devnum(udev, S_ISBLK(st.st_mode) ? 'b' : 'c', st.st_rdev); - if (!device) - return log_error_errno(errno, "Failed to get udev device from devnum %u:%u: %m", major(st.st_rdev), minor(st.st_rdev)); + r = udev_device_new_from_stat_rdev(udev, &st, &device); + if (r < 0) + return log_error_errno(r, "Failed to get udev device from devnum %u:%u: %m", major(st.st_rdev), minor(st.st_rdev)); while (d) { _cleanup_free_ char *match = NULL; diff --git a/src/shared/udev-util.c b/src/shared/udev-util.c index 29484226c6..a5ed8e8f6b 100644 --- a/src/shared/udev-util.c +++ b/src/shared/udev-util.c @@ -42,3 +42,26 @@ int udev_parse_config(void) { return 0; } + +int udev_device_new_from_stat_rdev(struct udev *udev, const struct stat *st, struct udev_device **ret) { + struct udev_device *nd; + char type; + + assert(udev); + assert(st); + assert(ret); + + if (S_ISBLK(st->st_mode)) + type = 'b'; + else if (S_ISCHR(st->st_mode)) + type = 'c'; + else + return -ENOTTY; + + nd = udev_device_new_from_devnum(udev, type, st->st_rdev); + if (!nd) + return -errno; + + *ret = nd; + return 0; +} diff --git a/src/shared/udev-util.h b/src/shared/udev-util.h index 0f270f7fbd..ce40f695e8 100644 --- a/src/shared/udev-util.h +++ b/src/shared/udev-util.h @@ -21,3 +21,5 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(struct udev_ctrl_msg*, udev_ctrl_msg_unref); DEFINE_TRIVIAL_CLEANUP_FUNC(struct udev_monitor*, udev_monitor_unref); int udev_parse_config(void); + +int udev_device_new_from_stat_rdev(struct udev *udev, const struct stat *st, struct udev_device **ret); From 159f1e7666ba49ff7d4d8f4f9ff4ae92c7905a05 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Jun 2018 23:05:20 +0200 Subject: [PATCH 12/22] core: split out early-boot preset logic into a function of its own --- src/core/manager.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 108110fee9..cc555e259b 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1482,6 +1482,29 @@ static void manager_setup_bus(Manager *m) { } } +static void manager_preset_all(Manager *m) { + int r; + + assert(m); + + if (m->first_boot <= 0) + return; + + if (!MANAGER_IS_SYSTEM(m)) + return; + + if (m->test_run_flags != 0) + return; + + /* If this is the first boot, and we are in the host system, then preset everything */ + r = unit_file_preset_all(UNIT_FILE_SYSTEM, 0, NULL, UNIT_FILE_PRESET_ENABLE_ONLY, NULL, 0); + if (r < 0) + log_full_errno(r == -EEXIST ? LOG_NOTICE : LOG_WARNING, r, + "Failed to populate /etc with preset unit settings, ignoring: %m"); + else + log_info("Populated /etc with preset unit settings."); +} + int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { int r; @@ -1505,19 +1528,7 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { if (r < 0) return r; - /* If this is the first boot, and we are in the host system, then preset everything */ - if (m->first_boot > 0 && - MANAGER_IS_SYSTEM(m) && - !m->test_run_flags) { - - r = unit_file_preset_all(UNIT_FILE_SYSTEM, 0, NULL, UNIT_FILE_PRESET_ENABLE_ONLY, NULL, 0); - if (r < 0) - log_full_errno(r == -EEXIST ? LOG_NOTICE : LOG_WARNING, r, - "Failed to populate /etc with preset unit settings, ignoring: %m"); - else - log_info("Populated /etc with preset unit settings."); - } - + manager_preset_all(m); lookup_paths_reduce(&m->lookup_paths); manager_build_unit_path_cache(m); From 62b0cbb358a1f48261efbbf0fb6b8fc704645491 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Jun 2018 23:05:39 +0200 Subject: [PATCH 13/22] core: use safe_fclose() where appropriate --- src/core/manager.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index cc555e259b..a0c404dc9d 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -3380,8 +3380,7 @@ int manager_reload(Manager *m) { r = q; } - fclose(f); - f = NULL; + f = safe_fclose(f); /* Re-register notify_fd as event source */ q = manager_setup_notify(m); From f0831ed2a03fcef582660be1c3b1a9f3e267e656 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Jun 2018 16:53:22 +0200 Subject: [PATCH 14/22] core: add a new unit method "catchup()" This is very similar to the existing unit method coldplug() but is called a bit later. The idea is that that coldplug() restores the unit state from before any prior reload/restart, i.e. puts the deserialized state in effect. The catchup() call is then called a bit later, to catch up with the system state for which we missed notifications while we were reloading. This is only really useful for mount, swap and device mount points were we should be careful to generate all missing unit state change events (i.e. call unit_notify() appropriately) for everything that happened while we were reloading. --- src/core/manager.c | 30 +++++++++++++++++++++++++++++- src/core/unit.c | 11 ++++++++--- src/core/unit.h | 17 ++++++++++------- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index a0c404dc9d..f90cc12910 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1350,7 +1350,9 @@ static void manager_coldplug(Manager *m) { assert(m); - /* Then, let's set up their initial state. */ + log_debug("Invoking unit coldplug() handlers…"); + + /* Let's place the units back into their deserialized state */ HASHMAP_FOREACH_KEY(u, k, m->units, i) { /* ignore aliases */ @@ -1363,6 +1365,26 @@ static void manager_coldplug(Manager *m) { } } +static void manager_catchup(Manager *m) { + Iterator i; + Unit *u; + char *k; + + assert(m); + + log_debug("Invoking unit catchup() handlers…"); + + /* Let's catch up on any state changes that happened while we were reloading/reexecing */ + HASHMAP_FOREACH_KEY(u, k, m->units, i) { + + /* ignore aliases */ + if (u->id != k) + continue; + + unit_catchup(u); + } +} + static void manager_build_unit_path_cache(Manager *m) { char **i; int r; @@ -1602,6 +1624,9 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { m->send_reloading_done = true; } + /* Let's finally catch up with any changes that took place while we were reloading/reexecing */ + manager_catchup(m); + return 0; } @@ -3414,6 +3439,9 @@ int manager_reload(Manager *m) { manager_recheck_journal(m); manager_recheck_dbus(m); + /* Let's finally catch up with any changes that took place while we were reloading/reexecing */ + manager_catchup(m); + /* Sync current state of bus names with our set of listening units */ q = manager_enqueue_sync_bus_names(m); if (q < 0 && r >= 0) diff --git a/src/core/unit.c b/src/core/unit.c index 7265f95c95..a82e5fd9eb 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2298,7 +2298,6 @@ static void unit_update_on_console(Unit *u) { manager_ref_console(u->manager); else manager_unref_console(u->manager); - } void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlags flags) { @@ -3730,8 +3729,7 @@ int unit_coldplug(Unit *u) { assert(u); - /* Make sure we don't enter a loop, when coldplugging - * recursively. */ + /* Make sure we don't enter a loop, when coldplugging recursively. */ if (u->coldplugged) return 0; @@ -3759,6 +3757,13 @@ int unit_coldplug(Unit *u) { return r; } +void unit_catchup(Unit *u) { + assert(u); + + if (UNIT_VTABLE(u)->catchup) + UNIT_VTABLE(u)->catchup(u); +} + static bool fragment_mtime_newer(const char *path, usec_t mtime, bool path_masked) { struct stat st; diff --git a/src/core/unit.h b/src/core/unit.h index 32bdd10643..9d9d94dd4e 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -437,10 +437,14 @@ typedef struct UnitVTable { * UNIT_STUB if no configuration could be found. */ int (*load)(Unit *u); - /* If a lot of units got created via enumerate(), this is - * where to actually set the state and call unit_notify(). */ + /* During deserialization we only record the intended state to return to. With coldplug() we actually put the + * deserialized state in effect. This is where unit_notify() should be called to start things up. */ int (*coldplug)(Unit *u); + /* This is called shortly after all units' coldplug() call was invoked. It's supposed to catch up state changes + * we missed so far (for example because they took place while we were reloading/reexecing) */ + void (*catchup)(Unit *u); + void (*dump)(Unit *u, FILE *f, const char *prefix); int (*start)(Unit *u); @@ -531,11 +535,9 @@ typedef struct UnitVTable { /* Returns true if the unit currently needs access to the console */ bool (*needs_console)(Unit *u); - /* This is called for each unit type and should be used to - * enumerate existing devices and load them. However, - * everything that is loaded here should still stay in - * inactive state. It is the job of the coldplug() call above - * to put the units into the initial state. */ + /* This is called for each unit type and should be used to enumerate units already existing in the system + * internally and load them. However, everything that is loaded here should still stay in inactive state. It is + * the job of the coldplug() call above to put the units into the initial state. */ void (*enumerate)(Manager *m); /* Type specific cleanups. */ @@ -687,6 +689,7 @@ void unit_serialize_item_format(Unit *u, FILE *f, const char *key, const char *v int unit_add_node_dependency(Unit *u, const char *what, bool wants, UnitDependency d, UnitDependencyMask mask); int unit_coldplug(Unit *u); +void unit_catchup(Unit *u); void unit_status_printf(Unit *u, const char *status, const char *unit_status_msg_format) _printf_(3, 0); void unit_status_emit_starting_stopping_reloading(Unit *u, JobType t); From 69ce73d18d474f3531e52ade69a2a4725466888f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Jun 2018 17:22:11 +0200 Subject: [PATCH 15/22] device: simplify device_found_to_string_many() a tiny bit No need to maintain a NULL marker at the end of the table if we know the size of the array anyway. --- src/core/device.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index e92b982439..21e90b8655 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -157,7 +157,6 @@ static const struct { { DEVICE_FOUND_UDEV_DB, "found-udev-db" }, { DEVICE_FOUND_MOUNT, "found-mount" }, { DEVICE_FOUND_SWAP, "found-swap" }, - {} }; static int device_found_to_string_many(DeviceFound flags, char **ret) { @@ -166,7 +165,7 @@ static int device_found_to_string_many(DeviceFound flags, char **ret) { assert(ret); - for (i = 0; device_found_map[i].name; i++) { + for (i = 0; i < ELEMENTSOF(device_found_map); i++) { if (!FLAGS_SET(flags, device_found_map[i].flag)) continue; @@ -196,7 +195,7 @@ static int device_found_from_string_many(const char *name, DeviceFound *ret) { if (r == 0) break; - for (i = 0; device_found_map[i].name; i++) + for (i = 0; i < ELEMENTSOF(device_found_map); i++) if (streq(word, device_found_map[i].name)) { f = device_found_map[i].flag; break; From 66f3fdbb073d8263b34bd65982a7e59d09086b23 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Jun 2018 17:28:32 +0200 Subject: [PATCH 16/22] core: rework how device units get set up This reworks how device units are "powered on". This makes sure that any device changes that might have happened while we were restarting/reloading will be noticed properly. For that we'll now properly serialize/deserialize both the device unit state and the device "found" flags, and restore these initially in the "coldplug" phase of the manager deserialization. While enumerating the udev devices during startup we'll put together a new "found" flags mask, which we'll the switch to in the "catchup" phase of the manager deserialization, which follows the "coldplug" phase. Note that during the "coldplug" phase no unit state change events are generated, which is different for the "catchall" phase which will do that. Thus we correctly make sure that the deserialized state won't pull in new deps, but any device's change while we were reloading would. Fixes: #8832 Replaces: #8675 --- src/core/device.c | 170 +++++++++++++++++++++++++--------------------- src/core/device.h | 21 +++--- 2 files changed, 103 insertions(+), 88 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 21e90b8655..e1f4ada87f 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -30,6 +30,7 @@ static const UnitActiveState state_translation_table[_DEVICE_STATE_MAX] = { }; static int device_dispatch_io(sd_event_source *source, int fd, uint32_t revents, void *userdata); +static void device_update_found_one(Device *d, DeviceFound found, DeviceFound mask); static void device_unset_sysfs(Device *d) { Hashmap *devices; @@ -101,6 +102,8 @@ static void device_init(Unit *u) { u->job_running_timeout = u->manager->default_timeout_start_usec; u->ignore_on_isolate = true; + + d->deserialized_state = _DEVICE_STATE_INVALID; } static void device_done(Unit *u) { @@ -130,33 +133,37 @@ static int device_coldplug(Unit *u) { assert(d); assert(d->state == DEVICE_DEAD); - /* This should happen only when we reexecute PID1 from an old version - * which didn't serialize d->found. In this case simply assume that the - * device was in plugged state right before we started reexecuting which - * might be a wrong assumption. */ - if (d->found == DEVICE_FOUND_UDEV_DB) - d->found = DEVICE_FOUND_UDEV; + /* First, let's put the deserialized state and found mask into effect, if we have it. */ - if (d->found & DEVICE_FOUND_UDEV) - /* If udev says the device is around, it's around */ - device_set_state(d, DEVICE_PLUGGED); - else if (d->found != DEVICE_NOT_FOUND && d->deserialized_state != DEVICE_PLUGGED) - /* If a device is found in /proc/self/mountinfo or - * /proc/swaps, and was not yet announced via udev, - * it's "tentatively" around. */ - device_set_state(d, DEVICE_TENTATIVE); + if (d->deserialized_state < 0 || + (d->deserialized_state == d->state && + d->deserialized_found == d->found)) + return 0; + d->found = d->deserialized_found; + device_set_state(d, d->deserialized_state); return 0; } +static void device_catchup(Unit *u) { + Device *d = DEVICE(u); + + assert(d); + + /* Second, let's update the state with the enumerated state if it's different */ + if (d->enumerated_found == d->found) + return; + + device_update_found_one(d, d->enumerated_found, DEVICE_FOUND_MASK); +} + static const struct { DeviceFound flag; const char *name; } device_found_map[] = { - { DEVICE_FOUND_UDEV, "found-udev" }, - { DEVICE_FOUND_UDEV_DB, "found-udev-db" }, - { DEVICE_FOUND_MOUNT, "found-mount" }, - { DEVICE_FOUND_SWAP, "found-swap" }, + { DEVICE_FOUND_UDEV, "found-udev" }, + { DEVICE_FOUND_MOUNT, "found-mount" }, + { DEVICE_FOUND_SWAP, "found-swap" }, }; static int device_found_to_string_many(DeviceFound flags, char **ret) { @@ -236,18 +243,6 @@ static int device_deserialize_item(Unit *u, const char *key, const char *value, assert(value); assert(fds); - /* The device was known at the time units were serialized but it's not - * anymore at the time units are deserialized. This happens when PID1 is - * re-executed after having switched to the new rootfs: devices were - * enumerated but udevd wasn't running yet thus the list of devices - * (handled by systemd) to initialize was empty. In such case we wait - * for the device events to be re-triggered by udev so device units are - * properly re-initialized. */ - if (d->found == DEVICE_NOT_FOUND) { - assert(d->sysfs == NULL); - return 0; - } - if (streq(key, "state")) { DeviceState state; @@ -258,9 +253,9 @@ static int device_deserialize_item(Unit *u, const char *key, const char *value, d->deserialized_state = state; } else if (streq(key, "found")) { - r = device_found_from_string_many(value, &d->found); + r = device_found_from_string_many(value, &d->deserialized_found); if (r < 0) - log_unit_debug(u, "Failed to parse found value, ignoring: %s", value); + log_unit_debug_errno(u, r, "Failed to parse found value, ignoring: %s", value); } else log_unit_debug(u, "Unknown serialization key: %s", key); @@ -438,8 +433,10 @@ static int device_setup_unit(Manager *m, struct udev_device *dev, const char *pa if (dev) { sysfs = udev_device_get_syspath(dev); - if (!sysfs) + if (!sysfs) { + log_debug("Couldn't get syspath from udev device, ignoring."); return 0; + } } r = unit_name_from_path(path, ".device", &e); @@ -448,17 +445,21 @@ static int device_setup_unit(Manager *m, struct udev_device *dev, const char *pa u = manager_get_unit(m, e); if (u) { - /* The device unit can still be present even if the device was unplugged: a mount unit can reference it hence - * preventing the GC to have garbaged it. That's desired since the device unit may have a dependency on the - * mount unit which was added during the loading of the later. */ - if (dev && DEVICE(u)->state == DEVICE_PLUGGED) { + /* The device unit can still be present even if the device was unplugged: a mount unit can reference it + * hence preventing the GC to have garbaged it. That's desired since the device unit may have a + * dependency on the mount unit which was added during the loading of the later. When the device is + * plugged the sysfs might not be initialized yet, as we serialize the device's state but do not + * serialize the sysfs path across reloads/reexecs. Hence, when coming back from a reload/restart we + * might have the state valid, but not the sysfs path. Hence, let's filter out conflicting devices, but + * let's accept devices in any state with no sysfs path set. */ - /* This unit is in plugged state: we're sure it's attached to a device. */ - if (!path_equal(DEVICE(u)->sysfs, sysfs)) { - log_unit_debug(u, "Dev %s appeared twice with different sysfs paths %s and %s", - e, DEVICE(u)->sysfs, sysfs); - return -EEXIST; - } + if (DEVICE(u)->state == DEVICE_PLUGGED && + DEVICE(u)->sysfs && + sysfs && + !path_equal(DEVICE(u)->sysfs, sysfs)) { + log_unit_debug(u, "Device %s appeared twice with different sysfs paths %s and %s, ignoring the latter.", + e, DEVICE(u)->sysfs, sysfs); + return -EEXIST; } delete = false; @@ -470,24 +471,26 @@ static int device_setup_unit(Manager *m, struct udev_device *dev, const char *pa delete = true; r = unit_new_for_name(m, sizeof(Device), e, &u); - if (r < 0) + if (r < 0) { + log_error_errno(r, "Failed to allocate device unit %s: %m", e); goto fail; + } unit_add_to_load_queue(u); } - /* If this was created via some dependency and has not - * actually been seen yet ->sysfs will not be + /* If this was created via some dependency and has not actually been seen yet ->sysfs will not be * initialized. Hence initialize it if necessary. */ if (sysfs) { r = device_set_sysfs(DEVICE(u), sysfs); - if (r < 0) + if (r < 0) { + log_error_errno(r, "Failed to set sysfs path %s for device unit %s: %m", sysfs, e); goto fail; + } (void) device_update_description(u, dev, path); - /* The additional systemd udev properties we only interpret - * for the main object */ + /* The additional systemd udev properties we only interpret for the main object */ if (main) (void) device_add_udev_wants(u, dev); } @@ -499,13 +502,11 @@ static int device_setup_unit(Manager *m, struct udev_device *dev, const char *pa device_upgrade_mount_deps(u); /* Note that this won't dispatch the load queue, the caller has to do that if needed and appropriate */ - unit_add_to_dbus_queue(u); + return 0; fail: - log_unit_warning_errno(u, r, "Failed to set up device unit: %m"); - if (delete) unit_free(u); @@ -584,45 +585,51 @@ static int device_process_new(Manager *m, struct udev_device *dev) { return 0; } -static void device_update_found_one(Device *d, DeviceFound found, DeviceFound mask) { - DeviceFound n, previous; - +static void device_found_changed(Device *d, DeviceFound previous, DeviceFound now) { assert(d); - n = (d->found & ~mask) | (found & mask); - if (n == d->found) - return; - - previous = d->found; - d->found = n; - - if (MANAGER_IS_RUNNING(UNIT(d)->manager)) - return; - /* Didn't exist before, but does now? if so, generate a new invocation ID for it */ - if (previous == DEVICE_NOT_FOUND && d->found != DEVICE_NOT_FOUND) + if (previous == DEVICE_NOT_FOUND && now != DEVICE_NOT_FOUND) (void) unit_acquire_invocation_id(UNIT(d)); - if (d->found & DEVICE_FOUND_UDEV) - /* When the device is known to udev we consider it - * plugged. */ + if (FLAGS_SET(now, DEVICE_FOUND_UDEV)) + /* When the device is known to udev we consider it plugged. */ device_set_state(d, DEVICE_PLUGGED); - else if (d->found != DEVICE_NOT_FOUND && (previous & DEVICE_FOUND_UDEV) == 0) - /* If the device has not been seen by udev yet, but is - * now referenced by the kernel, then we assume the + else if (now != DEVICE_NOT_FOUND && !FLAGS_SET(previous, DEVICE_FOUND_UDEV)) + /* If the device has not been seen by udev yet, but is now referenced by the kernel, then we assume the * kernel knows it now, and udev might soon too. */ device_set_state(d, DEVICE_TENTATIVE); else { - /* If nobody sees the device, or if the device was - * previously seen by udev and now is only referenced - * from the kernel, then we consider the device is - * gone, the kernel just hasn't noticed it yet. */ - + /* If nobody sees the device, or if the device was previously seen by udev and now is only referenced + * from the kernel, then we consider the device is gone, the kernel just hasn't noticed it yet. */ device_set_state(d, DEVICE_DEAD); device_unset_sysfs(d); } } +static void device_update_found_one(Device *d, DeviceFound found, DeviceFound mask) { + assert(d); + + if (MANAGER_IS_RUNNING(UNIT(d)->manager)) { + DeviceFound n, previous; + + /* When we are already running, then apply the new mask right-away, and trigger state changes + * right-away */ + + n = (d->found & ~mask) | (found & mask); + if (n == d->found) + return; + + previous = d->found; + d->found = n; + + device_found_changed(d, previous, n); + } else + /* We aren't running yet, let's apply the new mask to the shadow variable instead, which we'll apply as + * soon as we catch-up with the state. */ + d->enumerated_found = (d->enumerated_found & ~mask) | (found & mask); +} + static void device_update_found_by_sysfs(Manager *m, const char *sysfs, DeviceFound found, DeviceFound mask) { Device *d, *l, *n; @@ -824,7 +831,7 @@ static void device_enumerate(Manager *m) { continue; (void) device_process_new(m, dev); - device_update_found_by_sysfs(m, sysfs, DEVICE_FOUND_UDEV_DB, DEVICE_FOUND_UDEV_DB); + device_update_found_by_sysfs(m, sysfs, DEVICE_FOUND_UDEV, DEVICE_FOUND_UDEV); } return; @@ -995,7 +1002,11 @@ void device_found_node(Manager *m, const char *node, DeviceFound found, DeviceFo /* This is called whenever we find a device referenced in /proc/swaps or /proc/self/mounts. Such a device might * be mounted/enabled at a time where udev has not finished probing it yet, and we thus haven't learned about - * it yet. In this case we will set the device unit to "tentative" state. */ + * it yet. In this case we will set the device unit to "tentative" state. + * + * This takes a pair of DeviceFound flags parameters. The 'mask' parameter is a bit mask that indicates which + * bits of 'found' to copy into the per-device DeviceFound flags field. Thus, this function may be used to set + * and unset individual bits in a single call, while merging partially with previous state. */ if ((found & mask) != 0) { _cleanup_(udev_device_unrefp) struct udev_device *dev = NULL; @@ -1039,6 +1050,7 @@ const UnitVTable device_vtable = { .load = unit_load_fragment_and_dropin_optional, .coldplug = device_coldplug, + .catchup = device_catchup, .serialize = device_serialize, .deserialize_item = device_deserialize_item, diff --git a/src/core/device.h b/src/core/device.h index ab534ca24e..51b27e71b5 100644 --- a/src/core/device.h +++ b/src/core/device.h @@ -11,26 +11,29 @@ typedef struct Device Device; +/* A mask specifying where we have seen the device currently. This is a bitmask because the device might show up + * asynchronously from each other at various places. For example, in very common case a device might already be mounted + * before udev finished probing it (think: a script setting up a loopback block device, formatting it and mounting it + * in quick succession). Hence we need to track precisely where it is already visible and where not. */ typedef enum DeviceFound { - DEVICE_NOT_FOUND = 0, - DEVICE_FOUND_UDEV = 1 << 1, - DEVICE_FOUND_UDEV_DB = 1 << 2, - DEVICE_FOUND_MOUNT = 1 << 3, - DEVICE_FOUND_SWAP = 1 << 4, + DEVICE_NOT_FOUND = 0, + DEVICE_FOUND_UDEV = 1U << 1, /* The device has shown up in the udev database */ + DEVICE_FOUND_MOUNT = 1U << 2, /* The device has shown up in /proc/self/mountinfo */ + DEVICE_FOUND_SWAP = 1U << 3, /* The device has shown up in /proc/swaps */ + DEVICE_FOUND_MASK = DEVICE_FOUND_UDEV|DEVICE_FOUND_MOUNT|DEVICE_FOUND_SWAP, } DeviceFound; struct Device { Unit meta; char *sysfs; - DeviceFound found; - /* In order to be able to distinguish dependencies on - different device nodes we might end up creating multiple - devices for the same sysfs path. We chain them up here. */ + /* In order to be able to distinguish dependencies on different device nodes we might end up creating multiple + * devices for the same sysfs path. We chain them up here. */ LIST_FIELDS(struct Device, same_sysfs); DeviceState state, deserialized_state; + DeviceFound found, deserialized_found, enumerated_found; bool bind_mounts; }; From 244f80554940a6a08b6aafea033b5547eae74db4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Jun 2018 18:09:27 +0200 Subject: [PATCH 17/22] core: tighten when we unset the sysfs path of device units Make sure that whenever we enter "dead" state we unset the sysfs path, not just when we are changing to it due to "found" mask changes. --- src/core/device.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index e1f4ada87f..0615156820 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -121,6 +121,9 @@ static void device_set_state(Device *d, DeviceState state) { old_state = d->state; d->state = state; + if (state == DEVICE_DEAD) + device_unset_sysfs(d); + if (state != old_state) log_unit_debug(UNIT(d), "Changed %s -> %s", device_state_to_string(old_state), device_state_to_string(state)); @@ -599,12 +602,10 @@ static void device_found_changed(Device *d, DeviceFound previous, DeviceFound no /* If the device has not been seen by udev yet, but is now referenced by the kernel, then we assume the * kernel knows it now, and udev might soon too. */ device_set_state(d, DEVICE_TENTATIVE); - else { + else /* If nobody sees the device, or if the device was previously seen by udev and now is only referenced * from the kernel, then we consider the device is gone, the kernel just hasn't noticed it yet. */ device_set_state(d, DEVICE_DEAD); - device_unset_sysfs(d); - } } static void device_update_found_one(Device *d, DeviceFound found, DeviceFound mask) { From 04eb582acc203eab0bc5c2cc5e13986f16e09df0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Jun 2018 18:26:45 +0200 Subject: [PATCH 18/22] core: enumerate perpetual units in a separate per-unit-type method Previously the enumerate() callback defined for each unit type would do two things: 1. It would create perpetual units (i.e. -.slice, system.slice, -.mount and init.scope) 2. It would enumerate units from /proc/self/mountinfo, /proc/swaps and the udev database With this change these two parts are split into two seperate methods: enumerate() now only does #2, while enumerate_perpetual() is responsible for #1. Why make this change? Well, perpetual units should have a slightly different effect that those found through enumeration: as perpetual units should be up unconditionally, perpetually and thus never change state, they should also not pull in deps by their state changing, not even when the state is first set to active. Thus, their state is generally initialized through the per-device coldplug() method in similar fashion to the deserialized state from a previous run would be put into place. OTOH units found through regular enumeration should result in state changes (and thus pull in deps due to state changes), hence their state should be put in effect in the catchup() method instead. Hence, given this difference, let's also separate the functions, so that the rule is: 1. What is created in enumerate_perpetual() should be started in coldplug() 2. What is created in enumerate() should be started in catchup(). --- src/core/manager.c | 22 ++++++++++++++++++++-- src/core/mount.c | 15 ++++++--------- src/core/scope.c | 4 ++-- src/core/slice.c | 4 ++-- src/core/unit.h | 8 +++++++- 5 files changed, 37 insertions(+), 16 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index f90cc12910..04e0c6fe3b 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1322,13 +1322,30 @@ Manager* manager_free(Manager *m) { return mfree(m); } +static void manager_enumerate_perpetual(Manager *m) { + UnitType c; + + assert(m); + + /* Let's ask every type to load all units from disk/kernel that it might know */ + for (c = 0; c < _UNIT_TYPE_MAX; c++) { + if (!unit_type_supported(c)) { + log_debug("Unit type .%s is not supported on this system.", unit_type_to_string(c)); + continue; + } + + if (unit_vtable[c]->enumerate_perpetual) + unit_vtable[c]->enumerate_perpetual(m); + } +} + + static void manager_enumerate(Manager *m) { UnitType c; assert(m); - /* Let's ask every type to load all units from disk/kernel - * that it might know */ + /* Let's ask every type to load all units from disk/kernel that it might know */ for (c = 0; c < _UNIT_TYPE_MAX; c++) { if (!unit_type_supported(c)) { log_debug("Unit type .%s is not supported on this system.", unit_type_to_string(c)); @@ -1562,6 +1579,7 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds) { /* First, enumerate what we can from all config files */ dual_timestamp_get(m->timestamps + MANAGER_TIMESTAMP_UNITS_LOAD_START); + manager_enumerate_perpetual(m); manager_enumerate(m); dual_timestamp_get(m->timestamps + MANAGER_TIMESTAMP_UNITS_LOAD_FINISH); diff --git a/src/core/mount.c b/src/core/mount.c index 64115dc21b..dcc44657b2 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1671,7 +1671,7 @@ static int mount_get_timeout(Unit *u, usec_t *timeout) { return 1; } -static int synthesize_root_mount(Manager *m) { +static void mount_enumerate_perpetual(Manager *m) { Unit *u; int r; @@ -1683,8 +1683,10 @@ static int synthesize_root_mount(Manager *m) { u = manager_get_unit(m, SPECIAL_ROOT_MOUNT); if (!u) { r = unit_new_for_name(m, sizeof(Mount), SPECIAL_ROOT_MOUNT, &u); - if (r < 0) - return log_error_errno(r, "Failed to allocate the special " SPECIAL_ROOT_MOUNT " unit: %m"); + if (r < 0) { + log_error_errno(r, "Failed to allocate the special " SPECIAL_ROOT_MOUNT " unit: %m"); + return; + } } u->perpetual = true; @@ -1692,8 +1694,6 @@ static int synthesize_root_mount(Manager *m) { unit_add_to_load_queue(u); unit_add_to_dbus_queue(u); - - return 0; } static bool mount_is_mounted(Mount *m) { @@ -1707,10 +1707,6 @@ static void mount_enumerate(Manager *m) { assert(m); - r = synthesize_root_mount(m); - if (r < 0) - goto fail; - mnt_init_debug(0); if (!m->mount_monitor) { @@ -2001,6 +1997,7 @@ const UnitVTable mount_vtable = { .can_transient = true, + .enumerate_perpetual = mount_enumerate_perpetual, .enumerate = mount_enumerate, .shutdown = mount_shutdown, diff --git a/src/core/scope.c b/src/core/scope.c index 5db3296044..27ff545313 100644 --- a/src/core/scope.c +++ b/src/core/scope.c @@ -540,7 +540,7 @@ _pure_ static const char *scope_sub_state_to_string(Unit *u) { return scope_state_to_string(SCOPE(u)->state); } -static void scope_enumerate(Manager *m) { +static void scope_enumerate_perpetual(Manager *m) { Unit *u; int r; @@ -622,5 +622,5 @@ const UnitVTable scope_vtable = { .bus_set_property = bus_scope_set_property, .bus_commit_properties = bus_scope_commit_properties, - .enumerate = scope_enumerate, + .enumerate_perpetual = scope_enumerate_perpetual, }; diff --git a/src/core/slice.c b/src/core/slice.c index 71614e4b89..e8a7c9a585 100644 --- a/src/core/slice.c +++ b/src/core/slice.c @@ -326,7 +326,7 @@ static int slice_make_perpetual(Manager *m, const char *name, Unit **ret) { return 0; } -static void slice_enumerate(Manager *m) { +static void slice_enumerate_perpetual(Manager *m) { Unit *u; int r; @@ -383,7 +383,7 @@ const UnitVTable slice_vtable = { .bus_set_property = bus_slice_set_property, .bus_commit_properties = bus_slice_commit_properties, - .enumerate = slice_enumerate, + .enumerate_perpetual = slice_enumerate_perpetual, .status_message_formats = { .finished_start_job = { diff --git a/src/core/unit.h b/src/core/unit.h index 9d9d94dd4e..cff3825ddc 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -535,9 +535,15 @@ typedef struct UnitVTable { /* Returns true if the unit currently needs access to the console */ bool (*needs_console)(Unit *u); + /* Like the enumerate() callback further down, but only enumerates the perpetual units, i.e. all units that + * unconditionally exist and are always active. The main reason to keep both enumeration functions separate is + * philosophical: the state of perpetual units should be put in place by coldplug(), while the state of those + * discovered through regular enumeration should be put in place by catchup(), see below. */ + void (*enumerate_perpetual)(Manager *m); + /* This is called for each unit type and should be used to enumerate units already existing in the system * internally and load them. However, everything that is loaded here should still stay in inactive state. It is - * the job of the coldplug() call above to put the units into the initial state. */ + * the job of the catchup() call above to put the units into the discovered state. */ void (*enumerate)(Manager *m); /* Type specific cleanups. */ From b8b846d7b4c20bd1ac8c72e49e9d134ce8bd5213 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Jun 2018 20:18:47 +0200 Subject: [PATCH 19/22] tree-wide: fix a number of log calls that use %m but have no errno set This is mostly fall-out from d1a1f0aaf0d2f08c60d1e0d32e646439d99f58dc, however some cases are older bugs. There might be more issues lurking, this was a simple grep for "%m" across the tree, with all lines removed that mention "errno" at all. --- src/basic/conf-files.c | 2 +- src/basic/fs-util.c | 6 +++--- src/basic/selinux-util.c | 14 +++++++------- src/core/automount.c | 2 +- src/core/service.c | 2 +- src/journal/journald-stream.c | 4 ++-- src/network/netdev/netdev.c | 2 +- src/nspawn/nspawn.c | 2 +- src/shared/bus-unit-util.c | 6 ++++-- src/shared/conf-parser.c | 4 ++-- src/shared/dropin.c | 2 +- 11 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/basic/conf-files.c b/src/basic/conf-files.c index 2e65a6e74f..15a066da7f 100644 --- a/src/basic/conf-files.c +++ b/src/basic/conf-files.c @@ -349,7 +349,7 @@ int conf_files_cat(const char *root, const char *name) { assert(endswith(dir, "/")); r = strv_extendf(&dirs, "%s%s.d", dir, name); if (r < 0) - return log_error("Failed to build directory list: %m"); + return log_error_errno(r, "Failed to build directory list: %m"); } r = conf_files_list_strv(&files, ".conf", root, 0, (const char* const*) dirs); diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 6146c66782..ab6ccf7c86 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -1155,9 +1155,9 @@ int fsync_directory_of_file(int fd) { r = fd_get_path(fd, &path); if (r < 0) { - log_debug("Failed to query /proc/self/fd/%d%s: %m", - fd, - r == -EOPNOTSUPP ? ", ignoring" : ""); + log_debug_errno(r, "Failed to query /proc/self/fd/%d%s: %m", + fd, + r == -EOPNOTSUPP ? ", ignoring" : ""); if (r == -EOPNOTSUPP) /* If /proc is not available, we're most likely running in some diff --git a/src/basic/selinux-util.c b/src/basic/selinux-util.c index aba3e4f15d..61504a673a 100644 --- a/src/basic/selinux-util.c +++ b/src/basic/selinux-util.c @@ -79,7 +79,7 @@ int mac_selinux_init(void) { label_hnd = selabel_open(SELABEL_CTX_FILE, NULL, 0); if (!label_hnd) { - log_enforcing("Failed to initialize SELinux context: %m"); + log_enforcing_errno(errno, "Failed to initialize SELinux context: %m"); r = security_getenforce() == 1 ? -errno : 0; } else { char timespan[FORMAT_TIMESPAN_MAX]; @@ -189,7 +189,7 @@ int mac_selinux_apply(const char *path, const char *label) { assert(label); if (setfilecon(path, label) < 0) { - log_enforcing("Failed to set SELinux security context %s on path %s: %m", label, path); + log_enforcing_errno(errno, "Failed to set SELinux security context %s on path %s: %m", label, path); if (security_getenforce() > 0) return -errno; } @@ -349,12 +349,12 @@ int mac_selinux_create_file_prepare(const char *path, mode_t mode) { if (errno == ENOENT) return 0; - log_enforcing("Failed to determine SELinux security context for %s: %m", path); + log_enforcing_errno(errno, "Failed to determine SELinux security context for %s: %m", path); } else { if (setfscreatecon_raw(filecon) >= 0) return 0; /* Success! */ - log_enforcing("Failed to set SELinux security context %s for %s: %m", filecon, path); + log_enforcing_errno(errno, "Failed to set SELinux security context %s for %s: %m", filecon, path); } if (security_getenforce() > 0) @@ -385,7 +385,7 @@ int mac_selinux_create_socket_prepare(const char *label) { assert(label); if (setsockcreatecon(label) < 0) { - log_enforcing("Failed to set SELinux security context %s for sockets: %m", label); + log_enforcing_errno(errno, "Failed to set SELinux security context %s for sockets: %m", label); if (security_getenforce() == 1) return -errno; @@ -457,13 +457,13 @@ int mac_selinux_bind(int fd, const struct sockaddr *addr, socklen_t addrlen) { if (errno == ENOENT) goto skipped; - log_enforcing("Failed to determine SELinux security context for %s: %m", path); + log_enforcing_errno(errno, "Failed to determine SELinux security context for %s: %m", path); if (security_getenforce() > 0) return -errno; } else { if (setfscreatecon_raw(fcon) < 0) { - log_enforcing("Failed to set SELinux security context %s for %s: %m", fcon, path); + log_enforcing_errno(errno, "Failed to set SELinux security context %s for %s: %m", fcon, path); if (security_getenforce() > 0) return -errno; } else diff --git a/src/core/automount.c b/src/core/automount.c index 3e848c8085..0aba87b778 100644 --- a/src/core/automount.c +++ b/src/core/automount.c @@ -177,7 +177,7 @@ static int automount_verify(Automount *a) { r = unit_name_from_path(a->where, ".automount", &e); if (r < 0) - return log_unit_error(UNIT(a), "Failed to generate unit name from path: %m"); + return log_unit_error_errno(UNIT(a), r, "Failed to generate unit name from path: %m"); if (!unit_has_name(UNIT(a), e)) { log_unit_error(UNIT(a), "Where= setting doesn't match unit name. Refusing."); diff --git a/src/core/service.c b/src/core/service.c index db0c1dc397..2d43779cfb 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1947,7 +1947,7 @@ static void service_enter_start(Service *s) { /* There's no command line configured for the main command? Hmm, that is strange. This can only * happen if the configuration changes at runtime. In this case, let's enter a failure * state. */ - log_unit_error(UNIT(s), "There's no 'start' task anymore we could start: %m"); + log_unit_error(UNIT(s), "There's no 'start' task anymore we could start."); r = -ENXIO; goto fail; } diff --git a/src/journal/journald-stream.c b/src/journal/journald-stream.c index 93a0cc2288..4975aba016 100644 --- a/src/journal/journald-stream.c +++ b/src/journal/journald-stream.c @@ -766,8 +766,8 @@ int server_restore_streams(Server *s, FDSet *fds) { /* No file descriptor? Then let's delete the state file */ log_debug("Cannot restore stream file %s", de->d_name); if (unlinkat(dirfd(d), de->d_name, 0) < 0) - log_warning("Failed to remove /run/systemd/journal/streams/%s: %m", - de->d_name); + log_warning_errno(errno, "Failed to remove /run/systemd/journal/streams/%s: %m", + de->d_name); continue; } diff --git a/src/network/netdev/netdev.c b/src/network/netdev/netdev.c index a0b2697183..7ca570eee6 100644 --- a/src/network/netdev/netdev.c +++ b/src/network/netdev/netdev.c @@ -242,7 +242,7 @@ static int netdev_enslave_ready(NetDev *netdev, Link* link, sd_netlink_message_h r = sd_netlink_call_async(netdev->manager->rtnl, req, callback, link, 0, NULL); if (r < 0) - return log_netdev_error(netdev, "Could not send rtnetlink message: %m"); + return log_netdev_error_errno(netdev, r, "Could not send rtnetlink message: %m"); link_ref(link); diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 5792ae9587..7f11d66062 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -2930,7 +2930,7 @@ static int outer_child( if (l < 0) return log_error_errno(errno, "Failed to send cgroup mode: %m"); if (l != sizeof(arg_unified_cgroup_hierarchy)) { - log_error("Short write while sending cgroup mode: %m"); + log_error("Short write while sending cgroup mode."); return -EIO; } diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index 539a7b4d9d..64b7ac8d69 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -1156,8 +1156,10 @@ static int bus_append_execute_property(sd_bus_message *m, const char *field, con r = extract_first_word(&w, &path, ":", EXTRACT_DONT_COALESCE_SEPARATORS); if (r < 0) return log_error_errno(r, "Failed to parse argument: %m"); - if (r == 0) - return log_error("Failed to parse argument: %m"); + if (r == 0) { + log_error("Failed to parse argument: %s", p); + return -EINVAL; + } r = sd_bus_message_append(m, "(ss)", path, w); if (r < 0) diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index 4ad83b806b..b5ee07c9bd 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -303,8 +303,8 @@ int config_parse(const char *unit, /* Only log on request, except for ENOENT, * since we return 0 to the caller. */ if ((flags & CONFIG_PARSE_WARN) || errno == ENOENT) - log_full(errno == ENOENT ? LOG_DEBUG : LOG_ERR, - "Failed to open configuration file '%s': %m", filename); + log_full_errno(errno == ENOENT ? LOG_DEBUG : LOG_ERR, errno, + "Failed to open configuration file '%s': %m", filename); return errno == ENOENT ? 0 : -errno; } } diff --git a/src/shared/dropin.c b/src/shared/dropin.c index d9362f6919..0fc02914ac 100644 --- a/src/shared/dropin.c +++ b/src/shared/dropin.c @@ -210,7 +210,7 @@ static int unit_file_find_dirs( type = unit_name_to_type(name); if (type < 0) { - log_error("Failed to to derive unit type from unit name: %m"); + log_error("Failed to to derive unit type from unit name: %s", name); return -EINVAL; } From a7f8be01aa746eca06cea6970fe83bba99ef4a37 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Jun 2018 20:47:26 +0200 Subject: [PATCH 20/22] core: be a bit stricter when validating SYSTEMD_ALIAS udev props --- src/core/device.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 0615156820..915991bf90 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -579,10 +579,12 @@ static int device_process_new(Manager *m, struct udev_device *dev) { if (r < 0) return log_warning_errno(r, "Failed to add parse SYSTEMD_ALIAS for %s: %m", sysfs); - if (path_is_absolute(word)) - (void) device_setup_unit(m, dev, word, false); - else + if (!path_is_absolute(word)) log_warning("SYSTEMD_ALIAS for %s is not an absolute path, ignoring: %s", sysfs, word); + else if (!path_is_normalized(word)) + log_warning("SYSTEMD_ALIAS for %s is not a normalized path, ignoring: %s", sysfs, word); + else + (void) device_setup_unit(m, dev, word, false); } return 0; From cb209a0489ef13b88659f3a6cecd50f620869c75 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Jun 2018 20:57:08 +0200 Subject: [PATCH 21/22] swap: trivial log message improvements --- src/core/swap.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/core/swap.c b/src/core/swap.c index ab9057f29d..398eb02aef 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -1277,9 +1277,7 @@ static void swap_shutdown(Manager *m) { assert(m); m->swap_event_source = sd_event_source_unref(m->swap_event_source); - m->proc_swaps = safe_fclose(m->proc_swaps); - m->swaps_by_devnode = hashmap_free(m->swaps_by_devnode); } @@ -1292,9 +1290,9 @@ static void swap_enumerate(Manager *m) { m->proc_swaps = fopen("/proc/swaps", "re"); if (!m->proc_swaps) { if (errno == ENOENT) - log_debug("Not swap enabled, skipping enumeration"); + log_debug_errno(errno, "Not swap enabled, skipping enumeration."); else - log_error_errno(errno, "Failed to open /proc/swaps: %m"); + log_warning_errno(errno, "Failed to open /proc/swaps, ignoring: %m"); return; } From bb527e1137b6b529633f1f956dda5b83b2c27557 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 4 Jun 2018 21:00:44 +0200 Subject: [PATCH 22/22] update TODO --- TODO | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/TODO b/TODO index 533f9d87bd..4c4de2a1c8 100644 --- a/TODO +++ b/TODO @@ -24,6 +24,12 @@ Janitorial Clean-ups: Features: +* rework mount.c and swap.c to follow proper state enumeration/deserialization + semantics, like we do for device.c now + +* When reloading configuration PID 1 should reset all its properties to the + original defaults before calling parse_config() + * Add OnTimezoneChange= and OnTimeChange= stanzas to .timer units in order to schedule events based on time and timezone changes.