From 75a50eb0dd5acf90ea1e7ca1e81f35fe08c2c389 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 2 Jun 2021 10:31:41 +0200 Subject: [PATCH 1/9] pid1: make device_process_new() return void We never use the return value, and we really shouldn't, hence let's drop it. --- src/core/device.c | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index d34d48d0ff..b1b270808a 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -551,9 +551,10 @@ static int device_setup_unit(Manager *m, sd_device *dev, const char *path, bool (void) device_update_description(u, dev, path); - /* So the user wants the mount units to be bound to the device but a mount unit might has been seen by systemd - * before the device appears on its radar. In this case the device unit is partially initialized and includes - * the deps on the mount unit but at that time the "bind mounts" flag wasn't not present. Fix this up now. */ + /* So the user wants the mount units to be bound to the device but a mount unit might has been seen + * by systemd before the device appears on its radar. In this case the device unit is partially + * initialized and includes the deps on the mount unit but at that time the "bind mounts" flag wasn't + * present. Fix this up now. */ if (dev && device_is_bound_by_mounts(DEVICE(u), dev)) device_upgrade_mount_deps(u); @@ -566,7 +567,7 @@ fail: return r; } -static int device_process_new(Manager *m, sd_device *dev) { +static void device_process_new(Manager *m, sd_device *dev) { const char *sysfs, *dn, *alias; dev_t devnum; int r; @@ -574,12 +575,13 @@ static int device_process_new(Manager *m, sd_device *dev) { assert(m); if (sd_device_get_syspath(dev, &sysfs) < 0) - return 0; + return; - /* Add the main unit named after the sysfs path */ - r = device_setup_unit(m, dev, sysfs, true); - if (r < 0) - return r; + /* Add the main unit named after the sysfs path. If this one fails, don't bother with the rest, as + * this one shall be the main device unit the others just follow. (Compare with how + * device_following() is implemented, see below, which looks for the sysfs device.) */ + if (device_setup_unit(m, dev, sysfs, true) < 0) + return; /* Add an additional unit for the device node */ if (sd_device_get_devname(dev, &dn) >= 0) @@ -595,13 +597,11 @@ static int device_process_new(Manager *m, sd_device *dev) { if (PATH_STARTSWITH_SET(p, "/dev/block/", "/dev/char/")) continue; - /* Verify that the symlink in the FS actually belongs - * to this device. This is useful to deal with - * conflicting devices, e.g. when two disks want the - * same /dev/disk/by-label/xxx link because they have - * the same label. We want to make sure that the same - * device that won the symlink wins in systemd, so we - * check the device node major/minor */ + /* Verify that the symlink in the FS actually belongs to this device. This is useful + * to deal with conflicting devices, e.g. when two disks want the same + * /dev/disk/by-label/xxx link because they have the same label. We want to make sure + * that the same device that won the symlink wins in systemd, so we check the device + * node major/minor */ if (stat(p, &st) >= 0 && ((!S_ISBLK(st.st_mode) && !S_ISCHR(st.st_mode)) || st.st_rdev != devnum)) @@ -613,7 +613,7 @@ static int device_process_new(Manager *m, sd_device *dev) { /* Add additional units for all explicitly configured aliases */ if (sd_device_get_property_value(dev, "SYSTEMD_ALIAS", &alias) < 0) - return 0; + return; for (;;) { _cleanup_free_ char *word = NULL; @@ -622,9 +622,9 @@ static int device_process_new(Manager *m, sd_device *dev) { if (r == 0) break; if (r == -ENOMEM) - return log_oom(); + return (void) log_oom(); if (r < 0) - return log_device_warning_errno(dev, r, "Failed to parse SYSTEMD_ALIAS property: %m"); + return (void) log_device_warning_errno(dev, r, "Failed to parse SYSTEMD_ALIAS property, ignoring: %m"); if (!path_is_absolute(word)) log_device_warning(dev, "SYSTEMD_ALIAS is not an absolute path, ignoring: %s", word); @@ -633,8 +633,6 @@ static int device_process_new(Manager *m, sd_device *dev) { else (void) device_setup_unit(m, dev, word, false); } - - return 0; } static void device_found_changed(Device *d, DeviceFound previous, DeviceFound now) { @@ -859,7 +857,7 @@ static void device_enumerate(Manager *m) { if (!device_is_ready(dev)) continue; - (void) device_process_new(m, dev); + device_process_new(m, dev); if (sd_device_get_syspath(dev, &sysfs) < 0) continue; @@ -955,7 +953,7 @@ static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void * } else if (device_is_ready(dev)) { - (void) device_process_new(m, dev); + device_process_new(m, dev); r = swap_process_device_new(m, dev); if (r < 0) From e41db484c3f3762564ead0c85651e26156147cbc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 2 Jun 2021 10:35:23 +0200 Subject: [PATCH 2/9] pid1: shorten code a bit --- src/core/device.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index b1b270808a..84898d7236 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -122,8 +122,8 @@ static int device_load(Unit *u) { return r; if (!u->description) { - /* Generate a description based on the path, to be used until the - device is initialized properly */ + /* Generate a description based on the path, to be used until the device is initialized + properly */ r = unit_name_to_path(u->id, &u->description); if (r < 0) log_unit_debug_errno(u, r, "Failed to unescape name: %m"); @@ -963,7 +963,6 @@ static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void * /* The device is found now, set the udev found bit */ 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. */ @@ -1026,8 +1025,6 @@ static int validate_node(Manager *m, const char *node, sd_device **ret) { } void device_found_node(Manager *m, const char *node, DeviceFound found, DeviceFound mask) { - int r; - assert(m); assert(node); @@ -1052,8 +1049,7 @@ void device_found_node(Manager *m, const char *node, DeviceFound found, DeviceFo * under the name referenced in /proc/swaps or /proc/self/mountinfo. But first, let's validate if * everything is alright with the device node. */ - r = validate_node(m, node, &dev); - if (r <= 0) + if (validate_node(m, node, &dev) <= 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 c8ad151a5836631f17a5912e2e031e1896aa3cac Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 2 Jun 2021 10:36:53 +0200 Subject: [PATCH 3/9] pid1: make return value of device_remove_old() void too --- src/core/device.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 84898d7236..d188b0cd22 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -889,27 +889,24 @@ static void device_propagate_reload_by_sysfs(Manager *m, const char *sysfs) { } } -static int device_remove_old(Manager *m, sd_device *dev) { +static void device_remove_old_on_move(Manager *m, sd_device *dev) { _cleanup_free_ char *syspath_old = NULL, *e = NULL; const char *devpath_old; int r; r = sd_device_get_property_value(dev, "DEVPATH_OLD", &devpath_old); - if (r < 0) { - log_device_debug_errno(dev, r, "Failed to get DEVPATH_OLD= property on 'move' uevent, ignoring: %m"); - return 0; - } + if (r < 0) + return (void) log_device_debug_errno(dev, r, "Failed to get DEVPATH_OLD= property on 'move' uevent, ignoring: %m"); syspath_old = path_join("/sys", devpath_old); if (!syspath_old) - return log_oom(); + return (void) log_oom(); r = unit_name_from_path(syspath_old, ".device", &e); if (r < 0) - return log_device_error_errno(dev, r, "Failed to generate unit name from old device path: %m"); + return (void) log_device_error_errno(dev, r, "Failed to generate unit name from old device path: %m"); device_update_found_by_sysfs(m, syspath_old, 0, DEVICE_FOUND_UDEV|DEVICE_FOUND_MOUNT|DEVICE_FOUND_SWAP); - return 0; } static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void *userdata) { @@ -937,7 +934,7 @@ static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void * device_propagate_reload_by_sysfs(m, sysfs); if (action == SD_DEVICE_MOVE) - (void) device_remove_old(m, dev); + device_remove_old_on_move(m, dev); /* A change event can signal that a device is becoming ready, in particular if the device is using * the SYSTEMD_READY logic in udev so we need to reach the else block of the following if, even for From ad172d19d5ef8b5a3631a8484cc3d1a28dba26c2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 2 Jun 2021 15:29:29 +0200 Subject: [PATCH 4/9] pid1: reduce log noise generated by devices with overly long sysfs paths This basically does what 2c905207db37c691d4abef868165ad5ea2dd0f4f did for mount units Fixes: #16161 --- src/core/device.c | 31 +++++++++++++++++++++++++++++-- src/systemd/sd-messages.h | 4 ++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index d188b0cd22..03a2a9f22a 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -3,6 +3,8 @@ #include #include +#include "sd-messages.h" + #include "alloc-util.h" #include "bus-error.h" #include "dbus-device.h" @@ -12,6 +14,7 @@ #include "log.h" #include "parse-util.h" #include "path-util.h" +#include "ratelimit.h" #include "serialize.h" #include "stat-util.h" #include "string-util.h" @@ -497,8 +500,32 @@ static int device_setup_unit(Manager *m, sd_device *dev, const char *path, bool } r = unit_name_from_path(path, ".device", &e); - if (r < 0) - return log_device_error_errno(dev, r, "Failed to generate unit name from device path: %m"); + if (r < 0) { + /* Let's complain about overly long device names only at most once every 5s or so. This is + * something we should mention, since relevant devices are not manageable by systemd, but not + * flood the log about. */ + static RateLimit rate_limit = { + .interval = 5 * USEC_PER_SEC, + .burst = 1, + }; + + /* If we cannot convert a device name to a unit name then let's ignore the device. So far, + * devices with such long names weren't really the kind you want to manage with systemd + * anyway, hence this shouldn't be a problem. */ + + if (r == -ENAMETOOLONG) + return log_struct_errno( + ratelimit_below(&rate_limit) ? LOG_WARNING : LOG_DEBUG, r, + "MESSAGE_ID=" SD_MESSAGE_DEVICE_PATH_NOT_SUITABLE_STR, + "DEVICE=%s", path, + LOG_MESSAGE("Device path '%s' too long to fit into unit name, ignoring device.", path)); + + return log_struct_errno( + ratelimit_below(&rate_limit) ? LOG_WARNING : LOG_DEBUG, r, + "MESSAGE_ID=" SD_MESSAGE_DEVICE_PATH_NOT_SUITABLE_STR, + "DEVICE=%s", path, + LOG_MESSAGE("Failed to generate valid unit name from device path '%s', ignoring device: %m", path)); + } u = manager_get_unit(m, e); if (u) { diff --git a/src/systemd/sd-messages.h b/src/systemd/sd-messages.h index 97ba02ffa8..aee0ddb686 100644 --- a/src/systemd/sd-messages.h +++ b/src/systemd/sd-messages.h @@ -170,6 +170,10 @@ _SD_BEGIN_DECLARATIONS; SD_ID128_MAKE(1b,3b,b9,40,37,f0,4b,bf,81,02,8e,13,5a,12,d2,93) #define SD_MESSAGE_MOUNT_POINT_PATH_NOT_SUITABLE_STR \ SD_ID128_MAKE_STR(1b,3b,b9,40,37,f0,4b,bf,81,02,8e,13,5a,12,d2,93) +#define SD_MESSAGE_DEVICE_PATH_NOT_SUITABLE \ + SD_ID128_MAKE(01,01,90,13,8f,49,4e,29,a0,ef,66,69,74,95,31,aa) +#define SD_MESSAGE_DEVICE_PATH_NOT_SUITABLE_STR \ + SD_ID128_MAKE_STR(01,01,90,13,8f,49,4e,29,a0,ef,66,69,74,95,31,aa) #define SD_MESSAGE_NOBODY_USER_UNSUITABLE SD_ID128_MAKE(b4,80,32,5f,9c,39,4a,7b,80,2c,23,1e,51,a2,75,2c) #define SD_MESSAGE_NOBODY_USER_UNSUITABLE_STR \ From 9951c8df1e8c10fb97e3509f3adafaf5585c9fcb Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 2 Jun 2021 15:31:50 +0200 Subject: [PATCH 5/9] pid1: properly propagate errors from device_setup_unit() We want to propagate errors here, since we want to make dependent on the success of creating the main device unit the creation of the auxiliary device units. Thus if we suppress errors here we might end up in exotic corner cases in a situation were we create the auxiliary ("following") device units without the primary one. --- 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 03a2a9f22a..62a987dea7 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -493,10 +493,8 @@ static int device_setup_unit(Manager *m, sd_device *dev, const char *path, bool if (dev) { r = sd_device_get_syspath(dev, &sysfs); - if (r < 0) { - log_device_debug_errno(dev, r, "Couldn't get syspath from device, ignoring: %m"); - return 0; - } + if (r < 0) + return log_device_debug_errno(dev, r, "Couldn't get syspath from device, ignoring: %m"); } r = unit_name_from_path(path, ".device", &e); From 68695ce4d688022ff5b86a13155bd8aa8ec91d55 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 2 Jun 2021 15:34:03 +0200 Subject: [PATCH 6/9] pid1: eat up errors in device_update_found_by_name() We eat up all errors in the caller already, and rightly so. --- src/core/device.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 62a987dea7..cceeb1fe7e 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -721,7 +721,7 @@ static void device_update_found_by_sysfs(Manager *m, const char *sysfs, DeviceFo device_update_found_one(d, found, mask); } -static int device_update_found_by_name(Manager *m, const char *path, DeviceFound found, DeviceFound mask) { +static void device_update_found_by_name(Manager *m, const char *path, DeviceFound found, DeviceFound mask) { _cleanup_free_ char *e = NULL; Unit *u; int r; @@ -730,18 +730,17 @@ static int device_update_found_by_name(Manager *m, const char *path, DeviceFound assert(path); if (mask == 0) - return 0; + return; r = unit_name_from_path(path, ".device", &e); if (r < 0) - return log_error_errno(r, "Failed to generate unit name from device path: %m"); + return (void) log_debug_errno(r, "Failed to generate unit name from device path, ignoring: %m"); u = manager_get_unit(m, e); if (!u) - return 0; + return; device_update_found_one(DEVICE(u), found, mask); - return 0; } static bool device_is_ready(sd_device *dev) { From 4d94c74fae21f7ef02a5f14b8296a4522d6b51df Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 2 Jun 2021 15:34:34 +0200 Subject: [PATCH 7/9] pid1: downgrade if we can't make sense of the old device on MOVE uevent If the name of the old device didn#t work for us, we don't have to clean anything up, since we know for sure that there won't be a device unit for it. hence downgrade log message about it. --- 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 cceeb1fe7e..5ed5ceb290 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -928,7 +928,7 @@ static void device_remove_old_on_move(Manager *m, sd_device *dev) { r = unit_name_from_path(syspath_old, ".device", &e); if (r < 0) - return (void) log_device_error_errno(dev, r, "Failed to generate unit name from old device path: %m"); + return (void) log_device_debug_errno(dev, r, "Failed to generate unit name from old device path, ignoring: %m"); device_update_found_by_sysfs(m, syspath_old, 0, DEVICE_FOUND_UDEV|DEVICE_FOUND_MOUNT|DEVICE_FOUND_SWAP); } From 6aeb8c89bab2cf8055128dd091b9ad58cdeb71c2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 2 Jun 2021 15:48:14 +0200 Subject: [PATCH 8/9] pid1: make swap_process_new() void This matches device_process_new(): this function should not fail, since it just reacts to external events. --- src/core/swap.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/core/swap.c b/src/core/swap.c index ecf42ad119..fc781a3815 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -492,7 +492,7 @@ fail: return r; } -static int swap_process_new(Manager *m, const char *device, int prio, bool set_flags) { +static void swap_process_new(Manager *m, const char *device, int prio, bool set_flags) { _cleanup_(sd_device_unrefp) sd_device *d = NULL; const char *dn, *devlink; struct stat st, st_link; @@ -500,25 +500,22 @@ static int swap_process_new(Manager *m, const char *device, int prio, bool set_f assert(m); - r = swap_setup_unit(m, device, device, prio, set_flags); - if (r < 0) - return r; + if (swap_setup_unit(m, device, device, prio, set_flags) < 0) + return; /* If this is a block device, then let's add duplicates for * all other names of this block device */ if (stat(device, &st) < 0 || !S_ISBLK(st.st_mode)) - return 0; + return; r = sd_device_new_from_stat_rdev(&d, &st); - if (r < 0) { - log_full_errno(r == -ENOENT ? LOG_DEBUG : LOG_WARNING, r, - "Failed to allocate device for swap %s: %m", device); - return 0; - } + if (r < 0) + return (void) log_full_errno(r == -ENOENT ? LOG_DEBUG : LOG_WARNING, r, + "Failed to allocate device for swap %s: %m", device); /* Add the main device node */ if (sd_device_get_devname(d, &dn) >= 0 && !streq(dn, device)) - swap_setup_unit(m, dn, device, prio, set_flags); + (void) swap_setup_unit(m, dn, device, prio, set_flags); /* Add additional units for all symlinks */ FOREACH_DEVICE_DEVLINK(d, devlink) { @@ -535,10 +532,8 @@ static int swap_process_new(Manager *m, const char *device, int prio, bool set_f st_link.st_rdev != st.st_rdev)) continue; - swap_setup_unit(m, devlink, device, prio, set_flags); + (void) swap_setup_unit(m, devlink, device, prio, set_flags); } - - return 0; } static void swap_set_state(Swap *s, SwapState state) { From e82c6e8b6230b237c838f053d52baa3297668eaa Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 2 Jun 2021 15:49:10 +0200 Subject: [PATCH 9/9] pid1: don't choke on overly long device paths This mimics what we do for device units: if there's a device we cannot synthesize a good swap unit name for, then proceed without failure. --- src/core/swap.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/core/swap.c b/src/core/swap.c index fc781a3815..8aecc391e7 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -1426,13 +1426,14 @@ int swap_process_device_new(Manager *m, sd_device *dev) { assert(m); assert(dev); - r = sd_device_get_devname(dev, &dn); - if (r < 0) + if (sd_device_get_devname(dev, &dn) < 0) return 0; r = unit_name_from_path(dn, ".swap", &e); - if (r < 0) - return r; + if (r < 0) { + log_debug_errno(r, "Cannot convert device name '%s' to unit name, ignoring: %m", dn); + return 0; + } u = manager_get_unit(m, e); if (u)