From 3e61656fab869bb40f019c38c3347885238294de Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 28 Oct 2022 09:08:09 +0900 Subject: [PATCH 1/5] sd-id128: introduce id128_hash_ops_free --- src/libsystemd/sd-id128/id128-util.c | 1 + src/libsystemd/sd-id128/id128-util.h | 1 + 2 files changed, 2 insertions(+) diff --git a/src/libsystemd/sd-id128/id128-util.c b/src/libsystemd/sd-id128/id128-util.c index a27f4bf7427..a009a110a96 100644 --- a/src/libsystemd/sd-id128/id128-util.c +++ b/src/libsystemd/sd-id128/id128-util.c @@ -173,6 +173,7 @@ sd_id128_t id128_make_v4_uuid(sd_id128_t id) { } DEFINE_HASH_OPS(id128_hash_ops, sd_id128_t, id128_hash_func, id128_compare_func); +DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR(id128_hash_ops_free, sd_id128_t, id128_hash_func, id128_compare_func, free); int id128_get_product(sd_id128_t *ret) { sd_id128_t uuid; diff --git a/src/libsystemd/sd-id128/id128-util.h b/src/libsystemd/sd-id128/id128-util.h index 4b91a16bd16..887f443d693 100644 --- a/src/libsystemd/sd-id128/id128-util.h +++ b/src/libsystemd/sd-id128/id128-util.h @@ -27,6 +27,7 @@ int id128_write(const char *p, Id128FormatFlag f, sd_id128_t id); void id128_hash_func(const sd_id128_t *p, struct siphash *state); int id128_compare_func(const sd_id128_t *a, const sd_id128_t *b) _pure_; extern const struct hash_ops id128_hash_ops; +extern const struct hash_ops id128_hash_ops_free; sd_id128_t id128_make_v4_uuid(sd_id128_t id); From b15039425feba8f316fb306b75d96e2f0f0b82fa Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 28 Oct 2022 10:14:09 +0900 Subject: [PATCH 2/5] udevadm-trigger: allow to fallback without synthetic UUID only first time If a device is successfully triggered with synthetic UUID, then that means the kernel support it. Hence, it is not necessary to fallback without UUID for later devices. --- src/udev/udevadm-trigger.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/udev/udevadm-trigger.c b/src/udev/udevadm-trigger.c index 1d421064d78..cda31edd759 100644 --- a/src/udev/udevadm-trigger.c +++ b/src/udev/udevadm-trigger.c @@ -26,17 +26,20 @@ static bool arg_verbose = false; static bool arg_dry_run = false; static bool arg_quiet = false; static bool arg_uuid = false; +static bool arg_settle = false; static int exec_list( sd_device_enumerator *e, sd_device_action_t action, Hashmap *settle_hashmap) { - bool skip_uuid_logic = false; + int uuid_supported = -1; const char *action_str; sd_device *d; int r, ret = 0; + assert(e); + action_str = device_action_to_string(action); FOREACH_DEVICE_AND_SUBSYSTEM(e, d) { @@ -57,14 +60,14 @@ static int exec_list( /* Use the UUID mode if the user explicitly asked for it, or if --settle has been specified, * so that we can recognize our own uevent. */ - r = sd_device_trigger_with_uuid(d, action, (arg_uuid || settle_hashmap) && !skip_uuid_logic ? &id : NULL); - if (r == -EINVAL && !arg_uuid && settle_hashmap && !skip_uuid_logic) { + r = sd_device_trigger_with_uuid(d, action, (arg_uuid || arg_settle) && uuid_supported != 0 ? &id : NULL); + if (r == -EINVAL && !arg_uuid && arg_settle && uuid_supported < 0) { /* If we specified a UUID because of the settling logic, and we got EINVAL this might * be caused by an old kernel which doesn't know the UUID logic (pre-4.13). Let's try * if it works without the UUID logic then. */ r = sd_device_trigger(d, action); if (r != -EINVAL) - skip_uuid_logic = true; /* dropping the uuid stuff changed the return code, + uuid_supported = false; /* dropping the uuid stuff changed the return code, * hence don't bother next time */ } if (r < 0) { @@ -108,11 +111,14 @@ static int exec_list( continue; } + if (uuid_supported < 0) + uuid_supported = true; + /* If the user asked for it, write event UUID to stdout */ if (arg_uuid) printf(SD_ID128_UUID_FORMAT_STR "\n", SD_ID128_FORMAT_VAL(id)); - if (settle_hashmap) { + if (arg_settle) { _cleanup_free_ sd_id128_t *mid = NULL; _cleanup_free_ char *sp = NULL; @@ -285,7 +291,7 @@ int trigger_main(int argc, char *argv[], void *userdata) { _cleanup_(sd_event_unrefp) sd_event *event = NULL; _cleanup_hashmap_free_ Hashmap *settle_hashmap = NULL; usec_t ping_timeout_usec = 5 * USEC_PER_SEC; - bool settle = false, ping = false; + bool ping = false; int c, r; if (running_in_chroot() > 0) { @@ -389,7 +395,7 @@ int trigger_main(int argc, char *argv[], void *userdata) { break; } case 'w': - settle = true; + arg_settle = true; break; case ARG_NAME: { @@ -477,7 +483,7 @@ int trigger_main(int argc, char *argv[], void *userdata) { return log_error_errno(r, "Failed to add parent match '%s': %m", argv[optind]); } - if (settle) { + if (arg_settle) { settle_hashmap = hashmap_new(&path_hash_ops_free_free); if (!settle_hashmap) return log_oom(); From dfbd824a0b780310d7f865a6ea0d60434d924683 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 28 Oct 2022 10:21:57 +0900 Subject: [PATCH 3/5] udevadm-trigger: settle with synthetic UUID if the kernel support it If the kernel support synthetic UUID in uevent, then let's assume that the UUID is unique, and check only if the received UUID matches we specified. Partially fixes #25115. --- src/udev/udevadm-trigger.c | 82 +++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 42 deletions(-) diff --git a/src/udev/udevadm-trigger.c b/src/udev/udevadm-trigger.c index cda31edd759..3909fa237ca 100644 --- a/src/udev/udevadm-trigger.c +++ b/src/udev/udevadm-trigger.c @@ -11,10 +11,12 @@ #include "device-util.h" #include "fd-util.h" #include "fileio.h" +#include "id128-util.h" #include "parse-util.h" #include "path-util.h" #include "process-util.h" #include "set.h" +#include "static-destruct.h" #include "string-util.h" #include "strv.h" #include "udevadm.h" @@ -31,8 +33,9 @@ static bool arg_settle = false; static int exec_list( sd_device_enumerator *e, sd_device_action_t action, - Hashmap *settle_hashmap) { + Set **ret_settle_path_or_ids) { + _cleanup_set_free_ Set *settle_path_or_ids = NULL; int uuid_supported = -1; const char *action_str; sd_device *d; @@ -119,60 +122,62 @@ static int exec_list( printf(SD_ID128_UUID_FORMAT_STR "\n", SD_ID128_FORMAT_VAL(id)); if (arg_settle) { - _cleanup_free_ sd_id128_t *mid = NULL; - _cleanup_free_ char *sp = NULL; + if (uuid_supported) { + sd_id128_t *dup; - sp = strdup(syspath); - if (!sp) - return log_oom(); + dup = newdup(sd_id128_t, &id, 1); + if (!dup) + return log_oom(); - mid = newdup(sd_id128_t, &id, 1); - if (!d) - return log_oom(); + r = set_ensure_consume(&settle_path_or_ids, &id128_hash_ops_free, dup); + } else { + char *dup; - r = hashmap_put(settle_hashmap, sp, mid); + dup = strdup(syspath); + if (!dup) + return log_oom(); + + r = set_ensure_consume(&settle_path_or_ids, &path_hash_ops_free, dup); + } if (r < 0) return log_oom(); - - TAKE_PTR(sp); - TAKE_PTR(mid); } } + if (ret_settle_path_or_ids) + *ret_settle_path_or_ids = TAKE_PTR(settle_path_or_ids); + return ret; } static int device_monitor_handler(sd_device_monitor *m, sd_device *dev, void *userdata) { - Hashmap *settle_hashmap = ASSERT_PTR(userdata); - sd_id128_t *settle_id; + Set *settle_path_or_ids = * (Set**) ASSERT_PTR(userdata); const char *syspath; - char *k; + sd_id128_t id; int r; assert(dev); r = sd_device_get_syspath(dev, &syspath); if (r < 0) { - log_debug_errno(r, "Failed to get syspath of device event, ignoring: %m"); + log_device_debug_errno(dev, r, "Failed to get syspath of device event, ignoring: %m"); return 0; } - settle_id = hashmap_get2(settle_hashmap, syspath, (void**) &k); - if (!settle_id) { - log_debug("Got uevent for unexpected device '%s', ignoring.", syspath); - return 0; - } - if (!sd_id128_is_null(*settle_id)) { /* If this is SD_ID128_NULL then we are on pre-4.13 and have no UUID to check, hence don't */ - sd_id128_t event_id; + if (sd_device_get_trigger_uuid(dev, &id) >= 0) { + _cleanup_free_ sd_id128_t *saved = NULL; - r = sd_device_get_trigger_uuid(dev, &event_id); - if (r < 0) { - log_debug_errno(r, "Got uevent without synthetic UUID for device '%s', ignoring: %m", syspath); + saved = set_remove(settle_path_or_ids, &id); + if (!saved) { + log_device_debug(dev, "Got uevent not matching expected UUID, ignoring."); return 0; } + } else { + _cleanup_free_ char *saved = NULL; - if (!sd_id128_equal(event_id, *settle_id)) { - log_debug("Got uevent not matching expected UUID for device '%s', ignoring.", syspath); + saved = set_remove(settle_path_or_ids, syspath); + if (!saved) { + log_device_debug(dev, "Got uevent for unexpected device, ignoring."); return 0; } } @@ -181,12 +186,9 @@ static int device_monitor_handler(sd_device_monitor *m, sd_device *dev, void *us printf("settle %s\n", syspath); if (arg_uuid) - printf("settle " SD_ID128_UUID_FORMAT_STR "\n", SD_ID128_FORMAT_VAL(*settle_id)); + printf("settle " SD_ID128_UUID_FORMAT_STR "\n", SD_ID128_FORMAT_VAL(id)); - free(hashmap_remove(settle_hashmap, syspath)); - free(k); - - if (hashmap_isempty(settle_hashmap)) + if (set_isempty(settle_path_or_ids)) return sd_event_exit(sd_device_monitor_get_event(m), 0); return 0; @@ -289,7 +291,7 @@ int trigger_main(int argc, char *argv[], void *userdata) { _cleanup_(sd_device_enumerator_unrefp) sd_device_enumerator *e = NULL; _cleanup_(sd_device_monitor_unrefp) sd_device_monitor *m = NULL; _cleanup_(sd_event_unrefp) sd_event *event = NULL; - _cleanup_hashmap_free_ Hashmap *settle_hashmap = NULL; + _cleanup_set_free_ Set *settle_path_or_ids = NULL; usec_t ping_timeout_usec = 5 * USEC_PER_SEC; bool ping = false; int c, r; @@ -484,10 +486,6 @@ int trigger_main(int argc, char *argv[], void *userdata) { } if (arg_settle) { - settle_hashmap = hashmap_new(&path_hash_ops_free_free); - if (!settle_hashmap) - return log_oom(); - r = sd_event_default(&event); if (r < 0) return log_error_errno(r, "Failed to get default event: %m"); @@ -500,7 +498,7 @@ int trigger_main(int argc, char *argv[], void *userdata) { if (r < 0) return log_error_errno(r, "Failed to attach event to device monitor: %m"); - r = sd_device_monitor_start(m, device_monitor_handler, settle_hashmap); + r = sd_device_monitor_start(m, device_monitor_handler, &settle_path_or_ids); if (r < 0) return log_error_errno(r, "Failed to start device monitor: %m"); } @@ -525,11 +523,11 @@ int trigger_main(int argc, char *argv[], void *userdata) { assert_not_reached(); } - r = exec_list(e, action, settle_hashmap); + r = exec_list(e, action, arg_settle ? &settle_path_or_ids : NULL); if (r < 0) return r; - if (event && !hashmap_isempty(settle_hashmap)) { + if (!set_isempty(settle_path_or_ids)) { r = sd_event_loop(event); if (r < 0) return log_error_errno(r, "Event loop failed: %m"); From 1193448cb68e5a90cab027e16a093bbd367e9494 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 28 Oct 2022 09:06:02 +0900 Subject: [PATCH 4/5] udevadm-trigger: also check with the original syspath if device is renamed For older kernels that synthetic UUID is not supported, we need to also check the original device name, as udevd broadcasts uevent with new sysname. Fixes #25115. --- src/udev/udevadm-trigger.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/udev/udevadm-trigger.c b/src/udev/udevadm-trigger.c index 3909fa237ca..40ee5085a0b 100644 --- a/src/udev/udevadm-trigger.c +++ b/src/udev/udevadm-trigger.c @@ -176,6 +176,32 @@ static int device_monitor_handler(sd_device_monitor *m, sd_device *dev, void *us _cleanup_free_ char *saved = NULL; saved = set_remove(settle_path_or_ids, syspath); + if (!saved) { + const char *old_sysname; + + /* When the device is renamed, the new name is broadcast, and the old name is saved + * in INTERFACE_OLD. */ + + if (sd_device_get_property_value(dev, "INTERFACE_OLD", &old_sysname) >= 0) { + _cleanup_free_ char *dir = NULL, *old_syspath = NULL; + + r = path_extract_directory(syspath, &dir); + if (r < 0) { + log_device_debug_errno(dev, r, + "Failed to extract directory from '%s', ignoring: %m", + syspath); + return 0; + } + + old_syspath = path_join(dir, old_sysname); + if (!old_syspath) { + log_oom_debug(); + return 0; + } + + saved = set_remove(settle_path_or_ids, old_syspath); + } + } if (!saved) { log_device_debug(dev, "Got uevent for unexpected device, ignoring."); return 0; From ff4d2a09fd141474cb552d4b5bd5a53d9748a1b4 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 27 Oct 2022 05:48:05 +0900 Subject: [PATCH 5/5] test: use 'udevadm trigger --settle' even if device is renamed --- test/units/testsuite-17.02.sh | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/units/testsuite-17.02.sh b/test/units/testsuite-17.02.sh index ed3b39d074a..82f9fd1f629 100755 --- a/test/units/testsuite-17.02.sh +++ b/test/units/testsuite-17.02.sh @@ -61,9 +61,7 @@ EOF udevadm control --log-priority=debug --reload --timeout=30 -# FIXME(?): the 'add' uevent is broadcast as for 'foobar', instead of 'hoge'. Hence, we cannot use --settle here. -# See issue #25115. -udevadm trigger --action=add /sys/devices/virtual/net/hoge +udevadm trigger --action=add --settle /sys/devices/virtual/net/hoge udevadm wait --timeout=30 --settle /sys/devices/virtual/net/foobar assert_not_in "ID_RENAMING=" "$(udevadm info /sys/devices/virtual/net/foobar)" timeout 30 bash -c 'while [[ "$(systemctl show --property=ActiveState --value /sys/devices/virtual/net/hoge)" != "inactive" ]]; do sleep .5; done' @@ -71,7 +69,7 @@ timeout 30 bash -c 'while [[ "$(systemctl show --property=ActiveState --value /s timeout 30 bash -c 'while [[ "$(systemctl show --property=ActiveState --value /sys/devices/virtual/net/foobar)" != "active" ]]; do sleep .5; done' timeout 30 bash -c 'while [[ "$(systemctl show --property=ActiveState --value /sys/subsystem/net/devices/foobar)" != "active" ]]; do sleep .5; done' -udevadm trigger --action=add /sys/devices/virtual/net/foobar +udevadm trigger --action=add --settle /sys/devices/virtual/net/foobar udevadm wait --timeout=30 --settle /sys/devices/virtual/net/hoge assert_not_in "ID_RENAMING=" "$(udevadm info /sys/devices/virtual/net/hoge)" timeout 30 bash -c 'while [[ "$(systemctl show --property=ActiveState --value /sys/devices/virtual/net/hoge)" != "active" ]]; do sleep .5; done'