From 47e72170c1e808708b378ea83bf691d674b344ee Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 28 Apr 2022 20:41:34 +0900 Subject: [PATCH 01/11] core/device: use strv_consume() --- src/core/device.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 21d1b50c42ca8..81e4cdcd85684 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -408,11 +408,9 @@ static int device_add_udev_wants(Unit *u, sd_device *dev) { if (r < 0) return log_unit_error_errno(u, r, "Failed to add Wants= dependency: %m"); - r = strv_push(&added, k); + r = strv_consume(&added, TAKE_PTR(k)); if (r < 0) return log_oom(); - - k = NULL; } if (d->state != DEVICE_DEAD) From a7fb1f2eae3314c28d451822302283a7ab1bc1c0 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 28 Apr 2022 20:57:45 +0900 Subject: [PATCH 02/11] core/device: unit_name_from_path() does not return -ENAMETOOLONG anymore Follow-up for 1d0727e76fd5e9a07cc9991ec9a10ea1d78a99c7. --- src/core/device.c | 24 ++---------------------- 1 file changed, 2 insertions(+), 22 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 81e4cdcd85684..95dff82612d40 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -490,32 +490,12 @@ 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) { - /* 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)); - + if (r < 0) return log_struct_errno( - ratelimit_below(&rate_limit) ? LOG_WARNING : LOG_DEBUG, r, + LOG_WARNING, 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) { From dd309fcdb8a5c64e291a7186ed69965ad31134a7 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 28 Apr 2022 21:05:19 +0900 Subject: [PATCH 03/11] core/device: use _cleanup_ attribute at one more place --- src/core/device.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 95dff82612d40..17608901db59c 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -474,10 +474,10 @@ static void device_upgrade_mount_deps(Unit *u) { } static int device_setup_unit(Manager *m, sd_device *dev, const char *path, bool main) { + _cleanup_(unit_freep) Unit *new_unit = NULL; _cleanup_free_ char *e = NULL; const char *sysfs = NULL; - Unit *u = NULL; - bool delete; + Unit *u; int r; assert(m); @@ -515,19 +515,16 @@ static int device_setup_unit(Manager *m, sd_device *dev, const char *path, bool "Device %s appeared twice with different sysfs paths %s and %s, ignoring the latter.", e, DEVICE(u)->sysfs, sysfs); - delete = false; - /* Let's remove all dependencies generated due to udev properties. We'll re-add whatever is configured * now below. */ unit_remove_dependencies(u, UNIT_DEPENDENCY_UDEV); + } else { - delete = true; + r = unit_new_for_name(m, sizeof(Device), e, &new_unit); + if (r < 0) + return log_device_error_errno(dev, r, "Failed to allocate device unit %s: %m", e); - r = unit_new_for_name(m, sizeof(Device), e, &u); - if (r < 0) { - log_device_error_errno(dev, r, "Failed to allocate device unit %s: %m", e); - goto fail; - } + u = new_unit; unit_add_to_load_queue(u); } @@ -536,10 +533,8 @@ static int device_setup_unit(Manager *m, sd_device *dev, const char *path, bool * initialized. Hence initialize it if necessary. */ if (sysfs) { r = device_set_sysfs(DEVICE(u), sysfs); - if (r < 0) { - log_unit_error_errno(u, r, "Failed to set sysfs path %s: %m", sysfs); - goto fail; - } + if (r < 0) + return log_unit_error_errno(u, r, "Failed to set sysfs path %s: %m", sysfs); /* The additional systemd udev properties we only interpret for the main object */ if (main) @@ -555,13 +550,8 @@ static int device_setup_unit(Manager *m, sd_device *dev, const char *path, bool if (dev && device_is_bound_by_mounts(DEVICE(u), dev)) device_upgrade_mount_deps(u); + TAKE_PTR(new_unit); return 0; - -fail: - if (delete) - unit_free(u); - - return r; } static void device_process_new(Manager *m, sd_device *dev) { From 15345fc67739b8b62bcde55521a882d826faf801 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 28 Apr 2022 21:37:59 +0900 Subject: [PATCH 04/11] sd-device: introduce device_get_property_bool() --- src/libsystemd/sd-device/device-private.h | 1 + src/libsystemd/sd-device/sd-device.c | 21 +++++++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/libsystemd/sd-device/device-private.h b/src/libsystemd/sd-device/device-private.h index b6b36b6727093..5160f580069c0 100644 --- a/src/libsystemd/sd-device/device-private.h +++ b/src/libsystemd/sd-device/device-private.h @@ -17,6 +17,7 @@ static inline int device_new_from_watch_handle(sd_device **ret, int wd) { return device_new_from_watch_handle_at(ret, -1, wd); } +int device_get_property_bool(sd_device *device, const char *key); int device_get_device_id(sd_device *device, const char **ret); int device_get_devlink_priority(sd_device *device, int *ret); int device_get_watch_handle(sd_device *device); diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index c7f4c48d421e2..0bfcdcdf2d927 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -2007,6 +2007,20 @@ _public_ int sd_device_get_property_value(sd_device *device, const char *key, co return 0; } +int device_get_property_bool(sd_device *device, const char *key) { + const char *value; + int r; + + assert(device); + assert(key); + + r = sd_device_get_property_value(device, key, &value); + if (r < 0) + return r; + + return parse_boolean(value); +} + _public_ int sd_device_get_trigger_uuid(sd_device *device, sd_id128_t *ret) { const char *s; sd_id128_t id; @@ -2296,7 +2310,7 @@ _public_ int sd_device_trigger_with_uuid( _public_ int sd_device_open(sd_device *device, int flags) { _cleanup_close_ int fd = -1, fd2 = -1; - const char *devname, *subsystem = NULL, *val = NULL; + const char *devname, *subsystem = NULL; uint64_t q, diskseq = 0; struct stat st; dev_t devnum; @@ -2338,11 +2352,10 @@ _public_ int sd_device_open(sd_device *device, int flags) { if (FLAGS_SET(flags, O_PATH)) return TAKE_FD(fd); - r = sd_device_get_property_value(device, "ID_IGNORE_DISKSEQ", &val); + r = device_get_property_bool(device, "ID_IGNORE_DISKSEQ"); if (r < 0 && r != -ENOENT) return r; - - if (!val || parse_boolean(val) <= 0) { + if (r <= 0) { r = sd_device_get_diskseq(device, &diskseq); if (r < 0 && r != -ENOENT) return r; From 4212fa83d6ac6da2a5644ac7e83663a4944222e1 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 28 Apr 2022 21:43:19 +0900 Subject: [PATCH 05/11] core/device: use device_get_property_bool() --- src/core/device.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 17608901db59c..8cc46fb513646 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -9,6 +9,7 @@ #include "bus-error.h" #include "dbus-device.h" #include "dbus-unit.h" +#include "device-private.h" #include "device-util.h" #include "device.h" #include "log.h" @@ -438,20 +439,16 @@ static int device_add_udev_wants(Unit *u, sd_device *dev) { } static bool device_is_bound_by_mounts(Device *d, sd_device *dev) { - const char *bound_by; int r; assert(d); assert(dev); - if (sd_device_get_property_value(dev, "SYSTEMD_MOUNT_DEVICE_BOUND", &bound_by) >= 0) { - r = parse_boolean(bound_by); - if (r < 0) - log_device_warning_errno(dev, r, "Failed to parse SYSTEMD_MOUNT_DEVICE_BOUND='%s' udev property, ignoring: %m", bound_by); + r = device_get_property_bool(dev, "SYSTEMD_MOUNT_DEVICE_BOUND"); + if (r < 0 && r != -ENOENT) + log_device_warning_errno(dev, r, "Failed to parse SYSTEMD_MOUNT_DEVICE_BOUND= udev property, ignoring: %m"); - d->bind_mounts = r > 0; - } else - d->bind_mounts = false; + d->bind_mounts = r > 0; return d->bind_mounts; } @@ -706,8 +703,6 @@ static void device_update_found_by_name(Manager *m, const char *path, DeviceFoun } static bool device_is_ready(sd_device *dev) { - const char *ready; - assert(dev); if (device_is_renaming(dev) > 0) @@ -717,10 +712,7 @@ static bool device_is_ready(sd_device *dev) { if (sd_device_has_current_tag(dev, "systemd") <= 0) return false; - if (sd_device_get_property_value(dev, "SYSTEMD_READY", &ready) < 0) - return true; - - return parse_boolean(ready) != 0; + return device_get_property_bool(dev, "SYSTEMD_READY") != 0; } static Unit *device_following(Unit *u) { From f374631a56c967073719168c1877993c9ad4912a Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 28 Apr 2022 22:01:12 +0900 Subject: [PATCH 06/11] core/device: use udev_available() --- src/core/device.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 8cc46fb513646..6d662872ccb85 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -946,18 +946,6 @@ static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void * return 0; } -static bool device_supported(void) { - static int read_only = -1; - - /* If /sys is read-only we don't support device units, and any - * attempts to start one should fail immediately. */ - - if (read_only < 0) - read_only = path_is_read_only_fs("/sys"); - - return read_only <= 0; -} - static int validate_node(Manager *m, const char *node, sd_device **ret) { struct stat st; int r; @@ -1003,7 +991,7 @@ void device_found_node(Manager *m, const char *node, DeviceFound found, DeviceFo assert(m); assert(node); - if (!device_supported()) + if (!udev_available()) return; if (mask == 0) @@ -1073,7 +1061,7 @@ const UnitVTable device_vtable = { .enumerate = device_enumerate, .shutdown = device_shutdown, - .supported = device_supported, + .supported = udev_available, .status_message_formats = { .starting_stopping = { From 0e38cee8838a2aca9285557620718857e11969f5 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 28 Apr 2022 22:22:12 +0900 Subject: [PATCH 07/11] core/device: use sd_device_new_from_devname() to verify the device node --- src/core/device.c | 41 +++++++++++------------------------------ 1 file changed, 11 insertions(+), 30 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 6d662872ccb85..5d08697accfa5 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -946,45 +946,26 @@ static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void * return 0; } -static int validate_node(Manager *m, const char *node, sd_device **ret) { - struct stat st; +static int validate_node(const char *node, sd_device **ret) { + _cleanup_(sd_device_unrefp) sd_device *dev = NULL; int r; - 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")) { + r = sd_device_new_from_devname(&dev, node); + if (r == -ENODEV) { *ret = NULL; - return 0; /* bad! */ + return 0; /* good! (though missing) */ } + if (r < 0) + return r; /* 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_(sd_device_unrefp) sd_device *dev = NULL; - - r = sd_device_new_from_stat_rdev(&dev, &st); - 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! */ - } + *ret = TAKE_PTR(dev); + return 1; /* good! */ } void device_found_node(Manager *m, const char *node, DeviceFound found, DeviceFound mask) { @@ -1012,10 +993,10 @@ 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. */ - if (validate_node(m, node, &dev) <= 0) + if (validate_node(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); + (void) device_setup_unit(m, dev, node, false); /* 'dev' may be NULL. */ } /* Update the device unit's state, should it exist */ From 03a94b73c4779b6b5dc78f4e8b57d6b2ea0e7769 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 29 Apr 2022 22:27:53 +0900 Subject: [PATCH 08/11] core/device: device_found_node() does not accept DEVICE_FOUND_UDEV --- src/core/device.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/device.c b/src/core/device.c index 5d08697accfa5..3e0fd76f5e63b 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -971,6 +971,7 @@ static int validate_node(const char *node, sd_device **ret) { void device_found_node(Manager *m, const char *node, DeviceFound found, DeviceFound mask) { assert(m); assert(node); + assert(!FLAGS_SET(mask, DEVICE_FOUND_UDEV)); if (!udev_available()) return; From 42ebcebfef2e0dfcf1bb57dbec28b9aacd9bba92 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 28 Apr 2022 22:33:29 +0900 Subject: [PATCH 09/11] core/device: drop unused unit name generated from path --- 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 3e0fd76f5e63b..cc65b8580dffd 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -868,10 +868,13 @@ static void device_propagate_reload_by_sysfs(Manager *m, const char *sysfs) { } static void device_remove_old_on_move(Manager *m, sd_device *dev) { - _cleanup_free_ char *syspath_old = NULL, *e = NULL; + _cleanup_free_ char *syspath_old = NULL; const char *devpath_old; int r; + assert(m); + assert(dev); + r = sd_device_get_property_value(dev, "DEVPATH_OLD", &devpath_old); if (r < 0) return (void) log_device_debug_errno(dev, r, "Failed to get DEVPATH_OLD= property on 'move' uevent, ignoring: %m"); @@ -880,10 +883,6 @@ static void device_remove_old_on_move(Manager *m, sd_device *dev) { if (!syspath_old) return (void) log_oom(); - r = unit_name_from_path(syspath_old, ".device", &e); - if (r < 0) - 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 1363eeca946338735cec0d66020d1ed1d4eaedb0 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 28 Apr 2022 22:35:56 +0900 Subject: [PATCH 10/11] core/device: minor coding style updates --- src/core/device.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index cc65b8580dffd..9c3a75b4264e7 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -25,9 +25,9 @@ #include "unit.h" static const UnitActiveState state_translation_table[_DEVICE_STATE_MAX] = { - [DEVICE_DEAD] = UNIT_INACTIVE, + [DEVICE_DEAD] = UNIT_INACTIVE, [DEVICE_TENTATIVE] = UNIT_ACTIVATING, - [DEVICE_PLUGGED] = UNIT_ACTIVE, + [DEVICE_PLUGGED] = UNIT_ACTIVE, }; static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void *userdata); @@ -138,6 +138,7 @@ static int device_load(Unit *u) { static void device_set_state(Device *d, DeviceState state) { DeviceState old_state; + assert(d); if (d->state != state) @@ -557,6 +558,7 @@ static void device_process_new(Manager *m, sd_device *dev) { int r; assert(m); + assert(dev); if (sd_device_get_syspath(dev, &sysfs) < 0) return; @@ -887,12 +889,11 @@ static void device_remove_old_on_move(Manager *m, sd_device *dev) { } static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void *userdata) { + Manager *m = ASSERT_PTR(userdata); sd_device_action_t action; - Manager *m = userdata; const char *sysfs; int r; - assert(m); assert(dev); r = sd_device_get_syspath(dev, &sysfs); From 08300bb03505307db34b3289b3b1fca7f0475594 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 28 Apr 2022 22:49:58 +0900 Subject: [PATCH 11/11] core/device: use DEVICE_FOUND_MASK --- 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 9c3a75b4264e7..dd1f71d43c0da 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -885,7 +885,7 @@ static void device_remove_old_on_move(Manager *m, sd_device *dev) { if (!syspath_old) return (void) log_oom(); - device_update_found_by_sysfs(m, syspath_old, 0, DEVICE_FOUND_UDEV|DEVICE_FOUND_MOUNT|DEVICE_FOUND_SWAP); + device_update_found_by_sysfs(m, syspath_old, 0, DEVICE_FOUND_MASK); } static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void *userdata) { @@ -924,7 +924,7 @@ static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void * /* 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, 0, DEVICE_FOUND_UDEV|DEVICE_FOUND_MOUNT|DEVICE_FOUND_SWAP); + device_update_found_by_sysfs(m, sysfs, 0, DEVICE_FOUND_MASK); } else if (device_is_ready(dev)) {