From be73f6e35b81d6e534440c14b2cb40718a86c988 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 2 Feb 2025 13:30:26 +0900 Subject: [PATCH 1/3] udevadm: several cleanups around parse_device_action() - drop unnecessary one line function dump_device_action_table(), - make parse_device_action() log about invalid action string, - rename output argument of parse_device_action(). --- src/libsystemd/sd-device/device-private.c | 4 ---- src/libsystemd/sd-device/device-private.h | 1 - src/udev/udevadm-test-builtin.c | 6 ++---- src/udev/udevadm-test.c | 6 ++---- src/udev/udevadm-trigger.c | 6 ++---- src/udev/udevadm-util.c | 18 ++++++++---------- src/udev/udevadm-util.h | 2 +- 7 files changed, 15 insertions(+), 28 deletions(-) diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index 463eb7ea9e..00279ae01f 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -961,7 +961,3 @@ static const char* const device_action_table[_SD_DEVICE_ACTION_MAX] = { }; DEFINE_STRING_TABLE_LOOKUP(device_action, sd_device_action_t); - -void dump_device_action_table(void) { - DUMP_STRING_TABLE(device_action, sd_device_action_t, _SD_DEVICE_ACTION_MAX); -} diff --git a/src/libsystemd/sd-device/device-private.h b/src/libsystemd/sd-device/device-private.h index 4165c85023..aa42cbb2a1 100644 --- a/src/libsystemd/sd-device/device-private.h +++ b/src/libsystemd/sd-device/device-private.h @@ -77,4 +77,3 @@ int device_read_uevent_file(sd_device *device); int device_set_action(sd_device *device, sd_device_action_t a); sd_device_action_t device_action_from_string(const char *s) _pure_; const char* device_action_to_string(sd_device_action_t a) _const_; -void dump_device_action_table(void); diff --git a/src/udev/udevadm-test-builtin.c b/src/udev/udevadm-test-builtin.c index 382897efd4..13ecbe9773 100644 --- a/src/udev/udevadm-test-builtin.c +++ b/src/udev/udevadm-test-builtin.c @@ -45,10 +45,8 @@ static int parse_argv(int argc, char *argv[]) { switch (c) { case 'a': r = parse_device_action(optarg, &arg_action); - if (r < 0) - return log_error_errno(r, "Invalid action '%s'", optarg); - if (r == 0) - return 0; + if (r <= 0) + return r; break; case 'V': return print_version(); diff --git a/src/udev/udevadm-test.c b/src/udev/udevadm-test.c index c3f56d2d81..a6da337d7c 100644 --- a/src/udev/udevadm-test.c +++ b/src/udev/udevadm-test.c @@ -60,10 +60,8 @@ static int parse_argv(int argc, char *argv[]) { switch (c) { case 'a': r = parse_device_action(optarg, &arg_action); - if (r < 0) - return log_error_errno(r, "Invalid action '%s'", optarg); - if (r == 0) - return 0; + if (r <= 0) + return r; break; case 'N': arg_resolve_name_timing = resolve_name_timing_from_string(optarg); diff --git a/src/udev/udevadm-trigger.c b/src/udev/udevadm-trigger.c index 808c383e21..8ab4b49789 100644 --- a/src/udev/udevadm-trigger.c +++ b/src/udev/udevadm-trigger.c @@ -369,10 +369,8 @@ int trigger_main(int argc, char *argv[], void *userdata) { break; case 'c': r = parse_device_action(optarg, &action); - if (r < 0) - return log_error_errno(r, "Unknown action '%s'", optarg); - if (r == 0) - return 0; + if (r <= 0) + return r; break; case 's': r = sd_device_enumerator_add_match_subsystem(e, optarg, true); diff --git a/src/udev/udevadm-util.c b/src/udev/udevadm-util.c index 641adb4f97..858c23f586 100644 --- a/src/udev/udevadm-util.c +++ b/src/udev/udevadm-util.c @@ -10,6 +10,7 @@ #include "constants.h" #include "device-private.h" #include "path-util.h" +#include "string-table.h" #include "udev-ctrl.h" #include "udev-varlink.h" #include "udevadm-util.h" @@ -110,22 +111,19 @@ int find_device_with_action(const char *id, sd_device_action_t action, sd_device return 0; } -int parse_device_action(const char *str, sd_device_action_t *action) { - sd_device_action_t a; +int parse_device_action(const char *str, sd_device_action_t *ret) { assert(str); - assert(action); - if (streq(str, "help")) { - dump_device_action_table(); - return 0; - } + if (streq(str, "help")) + return DUMP_STRING_TABLE(device_action, sd_device_action_t, _SD_DEVICE_ACTION_MAX); - a = device_action_from_string(str); + sd_device_action_t a = device_action_from_string(str); if (a < 0) - return a; + return log_error_errno(a, "Invalid action '%s'.", str); - *action = a; + if (ret) + *ret = a; return 1; } diff --git a/src/udev/udevadm-util.h b/src/udev/udevadm-util.h index 0a8b31ada7..0846298502 100644 --- a/src/udev/udevadm-util.h +++ b/src/udev/udevadm-util.h @@ -5,6 +5,6 @@ int find_device(const char *id, const char *prefix, sd_device **ret); int find_device_with_action(const char *id, sd_device_action_t action, sd_device **ret); -int parse_device_action(const char *str, sd_device_action_t *action); +int parse_device_action(const char *str, sd_device_action_t *ret); int udev_ping(usec_t timeout, bool ignore_connection_failure); int search_rules_files(char * const *a, const char *root, char ***ret); From 8623980980d3798f26f23aa56c1491cfd6ceb7b2 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 2 Feb 2025 15:24:17 +0900 Subject: [PATCH 2/3] udevadm: introduce parse_resolve_name_timing() --- src/udev/udevadm-test.c | 7 +++---- src/udev/udevadm-util.c | 16 ++++++++++++++++ src/udev/udevadm-util.h | 3 +++ src/udev/udevadm-verify.c | 7 +++---- 4 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/udev/udevadm-test.c b/src/udev/udevadm-test.c index a6da337d7c..c00cc67d7c 100644 --- a/src/udev/udevadm-test.c +++ b/src/udev/udevadm-test.c @@ -64,10 +64,9 @@ static int parse_argv(int argc, char *argv[]) { return r; break; case 'N': - arg_resolve_name_timing = resolve_name_timing_from_string(optarg); - if (arg_resolve_name_timing < 0) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "--resolve-names= must be early, late or never"); + r = parse_resolve_name_timing(optarg, &arg_resolve_name_timing); + if (r <= 0) + return r; break; case 'D': { _cleanup_free_ char *p = NULL; diff --git a/src/udev/udevadm-util.c b/src/udev/udevadm-util.c index 858c23f586..bc7b277d91 100644 --- a/src/udev/udevadm-util.c +++ b/src/udev/udevadm-util.c @@ -12,6 +12,7 @@ #include "path-util.h" #include "string-table.h" #include "udev-ctrl.h" +#include "udev-rules.h" #include "udev-varlink.h" #include "udevadm-util.h" #include "unit-name.h" @@ -127,6 +128,21 @@ int parse_device_action(const char *str, sd_device_action_t *ret) { return 1; } +int parse_resolve_name_timing(const char *str, ResolveNameTiming *ret) { + assert(str); + + if (streq(str, "help")) + return DUMP_STRING_TABLE(resolve_name_timing, ResolveNameTiming, _RESOLVE_NAME_TIMING_MAX); + + ResolveNameTiming v = resolve_name_timing_from_string(optarg); + if (v < 0) + return log_error_errno(v, "--resolve-names= must be 'early', 'late', or 'never'."); + + if (ret) + *ret = v; + return 1; +} + static int udev_ping_via_ctrl(usec_t timeout_usec, bool ignore_connection_failure) { _cleanup_(udev_ctrl_unrefp) UdevCtrl *uctrl = NULL; int r; diff --git a/src/udev/udevadm-util.h b/src/udev/udevadm-util.h index 0846298502..cd0dcacf3c 100644 --- a/src/udev/udevadm-util.h +++ b/src/udev/udevadm-util.h @@ -3,8 +3,11 @@ #include "sd-device.h" +#include "udev-def.h" + int find_device(const char *id, const char *prefix, sd_device **ret); int find_device_with_action(const char *id, sd_device_action_t action, sd_device **ret); int parse_device_action(const char *str, sd_device_action_t *ret); +int parse_resolve_name_timing(const char *str, ResolveNameTiming *ret); int udev_ping(usec_t timeout, bool ignore_connection_failure); int search_rules_files(char * const *a, const char *root, char ***ret); diff --git a/src/udev/udevadm-verify.c b/src/udev/udevadm-verify.c index fb8cdee4f2..9286a632e8 100644 --- a/src/udev/udevadm-verify.c +++ b/src/udev/udevadm-verify.c @@ -77,10 +77,9 @@ static int parse_argv(int argc, char *argv[]) { case 'V': return print_version(); case 'N': - arg_resolve_name_timing = resolve_name_timing_from_string(optarg); - if (arg_resolve_name_timing < 0) - return log_error_errno(arg_resolve_name_timing, - "--resolve-names= takes \"early\" or \"never\""); + r = parse_resolve_name_timing(optarg, &arg_resolve_name_timing); + if (r <= 0) + return r; /* * In the verifier "late" has the effect of "never", * and "never" would generate irrelevant diagnostics, From 295741c0d120c35375a1a6c94e9b9c0808f0b85c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 2 Feb 2025 15:28:15 +0900 Subject: [PATCH 3/3] udevadm-verify: document '--resolve-names=late' and accept 'never' as is When '--resolve-names=late', systemd-udevd resolves user/group names during each event being processed, and does not verify names on parse. When '--resolve-names=never', systemd-udevd refuses any user/group names on parse. Hence, the parser of udev rules behaves diffrently. Let's not convert 'never' -> 'late' silently, and use the specified option as is. This also updates man page and shell completion for --resolve-names option. --- man/udevadm.xml | 25 ++++++++++++------------- shell-completion/bash/udevadm | 4 ++-- shell-completion/zsh/_udevadm | 5 +++-- src/udev/udevadm-verify.c | 9 +-------- test/meson.build | 2 +- test/units/TEST-17-UDEV.11.sh | 4 +++- 6 files changed, 22 insertions(+), 27 deletions(-) diff --git a/man/udevadm.xml b/man/udevadm.xml index 507ab49b48..021ae6d364 100644 --- a/man/udevadm.xml +++ b/man/udevadm.xml @@ -882,13 +882,11 @@ - Specify when udevadm should resolve names of users - and groups. When set to early (the - default), names will be resolved when the rules are - parsed. When set to late, names will - be resolved for every event. When set to - never, names will never be resolved - and all devices will be owned by root. + Specify when udevadm should resolve names of users and groups specified + in udev rules. When set to early (the default), names will be resolved when + the rules are parsed. When set to late, names will be resolved during the + event being processed. When set to never, names will never be resolved and + relevant udev rules will be ignored. @@ -976,13 +974,14 @@ - + - Specify when udevadm should resolve names of users - and groups. When set to early (the - default), names will be resolved when the rules are - parsed. When set to never, names will - never be resolved. + Specify when udevadm should resolve names of users and groups specified + in udev rules. When set to early (the default), names will be resolved when + the rules are parsed. When set to late, names will not be verified, as + systemd-udevd resolves names during each event being processed. When set to + never, names will never be resolved and relevant rules will be ignored. + diff --git a/shell-completion/bash/udevadm b/shell-completion/bash/udevadm index 02a025ff45..e9d11d32a3 100644 --- a/shell-completion/bash/udevadm +++ b/shell-completion/bash/udevadm @@ -258,7 +258,7 @@ _udevadm() { comps=$( udevadm test --action help ) ;; -N|--resolve-names) - comps='early late never' + comps=$( udevadm test --resolve-names help ) ;; -D|--extra-rules-dir) comps='' @@ -304,7 +304,7 @@ _udevadm() { if __contains_word "$prev" ${OPTS[VERIFY_ARG]}; then case $prev in -N|--resolve-names) - comps='early never' + comps=$( udevadm test --resolve-names help ) ;; --root) comps='' diff --git a/shell-completion/zsh/_udevadm b/shell-completion/zsh/_udevadm index 7572b09f67..ac112e751f 100644 --- a/shell-completion/zsh/_udevadm +++ b/shell-completion/zsh/_udevadm @@ -88,7 +88,8 @@ _udevadm_test(){ '(-)'{-h,--help}'[Show help]' \ '(-)'{-V,--version}'[Show package version]' \ '--action=[The action string.]:actions:(add change remove move online offline bind unbind)' \ - '--subsystem=[The subsystem string.]' \ + '(-N --resolve-names)'{-N,--resolve-names=}'[When to resolve names.]:resolve:(early late never)' \ + '--subsystem=[The subsystem string.]' \ '(-D --extra-rules-dir=)'{-D,--extra-rules-dir=}'[Also load rules from the directory.]' \ '(-v --verbose)'{-v,--verbose}'[Show verbose logs.]' \ '*::devpath:_files -P /sys/ -W /sys' @@ -119,7 +120,7 @@ _udevadm_verify(){ _arguments \ '(- *)'{-h,--help}'[Show help]' \ '(- *)'{-V,--version}'[Show package version]' \ - '(-N --resolve-names)'{-N+,--resolve-names=}'[When to resolve names.]:resolve:(early never)' \ + '(-N --resolve-names)'{-N,--resolve-names=}'[When to resolve names.]:resolve:(early late never)' \ '--root=[Operate on catalog hierarchy under specified directory]:directories:_directories' \ --no-summary'[Do not show summary.]' \ --no-style'[Ignore style issues.]' \ diff --git a/src/udev/udevadm-verify.c b/src/udev/udevadm-verify.c index 9286a632e8..ca2e0e472c 100644 --- a/src/udev/udevadm-verify.c +++ b/src/udev/udevadm-verify.c @@ -36,7 +36,7 @@ static int help(void) { "\n%sVerify udev rules files.%s\n\n" " -h --help Show this help\n" " -V --version Show package version\n" - " -N --resolve-names=early|never When to resolve names\n" + " -N --resolve-names=early|late|never When to resolve names\n" " --root=PATH Operate on an alternate filesystem root\n" " --no-summary Do not show summary\n" " --no-style Ignore style issues\n" @@ -80,13 +80,6 @@ static int parse_argv(int argc, char *argv[]) { r = parse_resolve_name_timing(optarg, &arg_resolve_name_timing); if (r <= 0) return r; - /* - * In the verifier "late" has the effect of "never", - * and "never" would generate irrelevant diagnostics, - * so map "never" to "late". - */ - if (arg_resolve_name_timing == RESOLVE_NAME_NEVER) - arg_resolve_name_timing = RESOLVE_NAME_LATE; break; case ARG_ROOT: r = parse_path_argument(optarg, /* suppress_root= */ true, &arg_root); diff --git a/test/meson.build b/test/meson.build index 92f184e3f5..f2fb5f79ce 100644 --- a/test/meson.build +++ b/test/meson.build @@ -136,7 +136,7 @@ if want_tests != 'false' test('udev-rules-check', exe, suite : 'udev', - args : ['verify', '--resolve-names=never', all_rules]) + args : ['verify', '--resolve-names=late', all_rules]) endif ############################################################ diff --git a/test/units/TEST-17-UDEV.11.sh b/test/units/TEST-17-UDEV.11.sh index d781db8d1e..33669a2a06 100755 --- a/test/units/TEST-17-UDEV.11.sh +++ b/test/units/TEST-17-UDEV.11.sh @@ -103,6 +103,8 @@ assert_0 -h assert_0 --help assert_0 -V assert_0 --version +assert_0 -N help +assert_0 --resolve-names help # unrecognized option '--unknown' assert_1 --unknown @@ -112,7 +114,7 @@ assert_1 -N assert_1 -N now # option '--resolve-names' requires an argument assert_1 --resolve-names -# --resolve-names= takes "early" or "never" +# --resolve-names= takes "early", "late", or "never" assert_1 --resolve-names=now # Failed to parse rules file ./nosuchfile: No such file or directory assert_1 ./nosuchfile