From 668e7c0cfdf8cfe0bf545507e8c6bc54e44cbc97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 10 Dec 2018 11:06:27 +0100 Subject: [PATCH 1/4] udevadm: improve error output when a device is not specified or specified wrong udevadm would dump help() output, instead of printing a message about what is wrong. That's just bad UX. Let's use a different message if the argument is missing, and a different one if it is invalid. Also, rework the code to separate the business logic from argument parsing. Let's not use "default:" in switch statements. This way, the compiler will warn us if we miss one of the cases. --- src/udev/udevadm-info.c | 225 +++++++++++++++++++++------------------- 1 file changed, 119 insertions(+), 106 deletions(-) diff --git a/src/udev/udevadm-info.c b/src/udev/udevadm-info.c index 1894362828f..4f7a4a388cc 100644 --- a/src/udev/udevadm-info.c +++ b/src/udev/udevadm-info.c @@ -22,6 +22,24 @@ #include "udevadm.h" #include "udevadm-util.h" +typedef enum ActionType { + ACTION_QUERY, + ACTION_ATTRIBUTE_WALK, + ACTION_DEVICE_ID_FILE, +} ActionType; + +typedef enum QueryType { + QUERY_NAME, + QUERY_PATH, + QUERY_SYMLINK, + QUERY_PROPERTY, + QUERY_ALL, +} QueryType; + +static bool arg_root = false; +static bool arg_export = false; +static const char *arg_export_prefix = NULL; + static bool skip_attribute(const char *name) { static const char* const skip[] = { "uevent", @@ -106,7 +124,7 @@ static int print_device_chain(sd_device *device) { return 0; } -static void print_record(sd_device *device) { +static int print_record(sd_device *device) { const char *str, *val; int i; @@ -126,6 +144,7 @@ static void print_record(sd_device *device) { printf("E: %s=%s\n", str, val); printf("\n"); + return 0; } static int stat_device(const char *name, bool export, const char *prefix) { @@ -223,8 +242,75 @@ static void cleanup_db(void) { cleanup_dir(dir5, 0, 1); } -static int help(void) { +static int query_device(QueryType query, sd_device* device) { + int r; + assert(device); + + switch(query) { + case QUERY_NAME: { + const char *node; + + r = sd_device_get_devname(device, &node); + if (r < 0) + return log_error_errno(r, "No device node found: %m"); + + if (arg_root) + printf("%s\n", node); + else + printf("%s\n", node + STRLEN("/dev/")); + return 0; + } + + case QUERY_SYMLINK: { + const char *devlink; + bool first = true; + + FOREACH_DEVICE_DEVLINK(device, devlink) { + if (!first) + printf(" "); + if (arg_root) + printf("%s", devlink); + else + printf("%s", devlink + STRLEN("/dev/")); + + first = false; + } + printf("\n"); + return 0; + } + + case QUERY_PATH: { + const char *devpath; + + r = sd_device_get_devpath(device, &devpath); + if (r < 0) + return log_error_errno(r, "Failed to get device path: %m"); + + printf("%s\n", devpath); + return 0; + } + + case QUERY_PROPERTY: { + const char *key, *value; + + FOREACH_DEVICE_PROPERTY(device, key, value) + if (arg_export) + printf("%s%s='%s'\n", strempty(arg_export_prefix), key, value); + else + printf("%s=%s\n", key, value); + return 0; + } + + case QUERY_ALL: + return print_record(device); + } + + assert_not_reached("unknown query type"); + return 0; +} + +static int help(void) { printf("%s info [OPTIONS] [DEVPATH|FILE]\n\n" "Query sysfs or the udev database.\n\n" " -h --help Print this message\n" @@ -252,9 +338,7 @@ static int help(void) { int info_main(int argc, char *argv[], void *userdata) { _cleanup_(sd_device_unrefp) sd_device *device = NULL; - bool root = false, export = false; _cleanup_free_ char *name = NULL; - const char *export_prefix = NULL; int c, r; static const struct option options[] = { @@ -273,19 +357,8 @@ int info_main(int argc, char *argv[], void *userdata) { {} }; - enum action_type { - ACTION_QUERY, - ACTION_ATTRIBUTE_WALK, - ACTION_DEVICE_ID_FILE, - } action = ACTION_QUERY; - - enum query_type { - QUERY_NAME, - QUERY_PATH, - QUERY_SYMLINK, - QUERY_PROPERTY, - QUERY_ALL, - } query = QUERY_ALL; + ActionType action = ACTION_QUERY; + QueryType query = QUERY_ALL; while ((c = getopt_long(argc, argv, "aced:n:p:q:rxP:RVh", options, NULL)) >= 0) switch (c) { @@ -327,7 +400,7 @@ int info_main(int argc, char *argv[], void *userdata) { } break; case 'r': - root = true; + arg_root = true; break; case 'd': action = ACTION_DEVICE_ID_FILE; @@ -344,10 +417,10 @@ int info_main(int argc, char *argv[], void *userdata) { cleanup_db(); return 0; case 'x': - export = true; + arg_export = true; break; case 'P': - export_prefix = optarg; + arg_export_prefix = optarg; break; case 'V': return print_version(); @@ -359,95 +432,35 @@ int info_main(int argc, char *argv[], void *userdata) { assert_not_reached("Unknown option"); } - switch (action) { - case ACTION_QUERY: - if (!device) { - if (!argv[optind]) { - help(); - return -EINVAL; - } - r = find_device(argv[optind], NULL, &device); - if (r < 0) - return log_error_errno(r, "Unknown device, --name=, --path=, or absolute path in /dev/ or /sys expected: %m"); - } + if (IN_SET(action, ACTION_QUERY, ACTION_ATTRIBUTE_WALK) && + !device) { + /* A device argument is required. It may be an option or a positional arg. */ - switch(query) { - case QUERY_NAME: { - const char *node; + if (!argv[optind]) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "A device name or path is required"); - r = sd_device_get_devname(device, &node); - if (r < 0) - return log_error_errno(r, "no device node found: %m"); - - if (root) - printf("%s\n", node); - else - printf("%s\n", node + STRLEN("/dev/")); - break; - } - case QUERY_SYMLINK: { - const char *devlink; - bool first = true; - - FOREACH_DEVICE_DEVLINK(device, devlink) { - if (!first) - printf(" "); - if (root) - printf("%s", devlink); - else - printf("%s", devlink + STRLEN("/dev/")); - - first = false; - } - printf("\n"); - break; - } - case QUERY_PATH: { - const char *devpath; - - r = sd_device_get_devpath(device, &devpath); - if (r < 0) - return log_error_errno(r, "Failed to get device path: %m"); - - printf("%s\n", devpath); - return 0; - } - case QUERY_PROPERTY: { - const char *key, *value; - - FOREACH_DEVICE_PROPERTY(device, key, value) - if (export) - printf("%s%s='%s'\n", strempty(export_prefix), key, value); - else - printf("%s=%s\n", key, value); - - break; - } - case QUERY_ALL: - print_record(device); - break; - default: - assert_not_reached("unknown query type"); - } - break; - case ACTION_ATTRIBUTE_WALK: - if (!device && argv[optind]) { - r = find_device(argv[optind], NULL, &device); - if (r < 0) - return log_error_errno(r, "Unknown device, absolute path in /dev/ or /sys expected: %m"); - } - if (!device) { - log_error("Unknown device, --name=, --path=, or absolute path in /dev/ or /sys expected."); - return -EINVAL; - } - print_device_chain(device); - break; - case ACTION_DEVICE_ID_FILE: - r = stat_device(name, export, export_prefix); + r = find_device(argv[optind], NULL, &device); + if (r == -EINVAL) + return log_error_errno(r, "Bad argument \"%s\", an absolute path in /dev/ or /sys expected: %m", + argv[optind]); if (r < 0) - return r; - break; + return log_error_errno(r, "Unknown device \"%s\": %m", argv[optind]); } - return 0; + switch (action) { + case ACTION_QUERY: + assert(device); + return query_device(query, device); + + case ACTION_ATTRIBUTE_WALK: + assert(device); + return print_device_chain(device); + + case ACTION_DEVICE_ID_FILE: + assert(name); + return stat_device(name, arg_export, arg_export_prefix); + } + + assert_not_reached("Unknown action"); } From d539f791769988f62a780e6fce2d4a77177d64ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 11 Dec 2018 07:34:45 +0100 Subject: [PATCH 2/4] udevadm: use path_startswith and shorten code a bit --- src/udev/udevadm-info.c | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/src/udev/udevadm-info.c b/src/udev/udevadm-info.c index 4f7a4a388cc..ff4cf4ec457 100644 --- a/src/udev/udevadm-info.c +++ b/src/udev/udevadm-info.c @@ -80,7 +80,7 @@ static void print_all_attributes(sd_device *device, const char *key) { printf(" %s{%s}==\"%s\"\n", key, name, value); } - printf("\n"); + puts(""); } static int print_device_chain(sd_device *device) { @@ -131,19 +131,23 @@ static int print_record(sd_device *device) { (void) sd_device_get_devpath(device, &str); printf("P: %s\n", str); - if (sd_device_get_devname(device, &str) >= 0) - printf("N: %s\n", str + STRLEN("/dev/")); + if (sd_device_get_devname(device, &str) >= 0) { + assert_se(val = path_startswith(str, "/dev/")); + printf("N: %s\n", val); + } if (device_get_devlink_priority(device, &i) >= 0) printf("L: %i\n", i); - FOREACH_DEVICE_DEVLINK(device, str) - printf("S: %s\n", str + STRLEN("/dev/")); + FOREACH_DEVICE_DEVLINK(device, str) { + assert_se(val = path_startswith(str, "/dev/")); + printf("S: %s\n", val); + } FOREACH_DEVICE_PROPERTY(device, str, val) printf("E: %s=%s\n", str, val); - printf("\n"); + puts(""); return 0; } @@ -255,28 +259,22 @@ static int query_device(QueryType query, sd_device* device) { if (r < 0) return log_error_errno(r, "No device node found: %m"); - if (arg_root) - printf("%s\n", node); - else - printf("%s\n", node + STRLEN("/dev/")); + if (!arg_root) + assert_se(node = path_startswith(node, "/dev/")); + printf("%s\n", node); return 0; } case QUERY_SYMLINK: { - const char *devlink; - bool first = true; + const char *devlink, *prefix = ""; FOREACH_DEVICE_DEVLINK(device, devlink) { - if (!first) - printf(" "); - if (arg_root) - printf("%s", devlink); - else - printf("%s", devlink + STRLEN("/dev/")); - - first = false; + if (!arg_root) + assert_se(devlink = path_startswith(devlink, "/dev/")); + printf("%s%s", prefix, devlink); + prefix = " "; } - printf("\n"); + puts(""); return 0; } From b6854081ffb26c32a8d1440346f9ee5b9d2f1e57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 10 Dec 2018 11:46:21 +0100 Subject: [PATCH 3/4] udevadm: allow a .device unit to be specified for query and trigger This is convenient when working with device units in systemd. Instead of converting the systemd unit name to a path to feed to udevadm, udevadm info|trigger can be called directly on the unit name. The man page is reworked a bit to describe the modern syntax with positional arguments first. It's just simpler to use than the positional options. --- man/udevadm.xml | 53 ++++++++++++++++++++--------------------- src/udev/udevadm-info.c | 2 +- src/udev/udevadm-util.c | 25 ++++++++++++++----- 3 files changed, 46 insertions(+), 34 deletions(-) diff --git a/man/udevadm.xml b/man/udevadm.xml index a63a92cef26..ac3198b1f87 100644 --- a/man/udevadm.xml +++ b/man/udevadm.xml @@ -76,39 +76,40 @@ udevadm info <arg choice="opt"><replaceable>options</replaceable></arg> - <arg choice="opt"><replaceable>devpath</replaceable>|<replaceable>file</replaceable></arg> + <arg choice="opt"><replaceable>devpath</replaceable>|<replaceable>file</replaceable>|<replaceable>unit</replaceable></arg> - Queries the udev database for device information - stored in the udev database. It can also query the properties - of a device from its sysfs representation to help creating udev - rules that match this device. + Query the udev database for device information. + + A positional argument should be used to specify a device. It may be a device name (in which case + it must start with /dev/), a sys path (in which case it must start with + /sys/), or a systemd device unit name (in which case it must end with + .device, see + systemd.device5). + + - Query the database for the specified type of device - data. It needs the or - to identify the specified device. + Query the database for the specified type of device data. Valid TYPEs are: name, symlink, path, property, all. + - The /sys path of the device to - query, e.g. - /sys/class/block/sda. - Note that this option usually is not very useful, since - udev can guess the type of the - argument, so udevadm info - --path=/class/block/sda is equivalent to - udevadm info /sys/class/block/sda. + The /sys path of the device to query, e.g. + /sys/class/block/sda. This option is an alternative to + the positional argument with a /sys/ prefix. udevadm info + --path=/class/block/sda is equivalent to udevadm info + /sys/class/block/sda. @@ -116,11 +117,9 @@ The name of the device node or a symlink to query, - e.g. /dev/sda. - Note that this option usually is not very useful, since - udev can guess the type of the - argument, so udevadm info --name=sda is - equivalent to udevadm info /dev/sda. + e.g. /dev/sda. This option is an alternative to the + positional argument with a /dev/ prefix. udevadm info + --name=sda is equivalent to udevadm info /dev/sda. @@ -179,17 +178,17 @@ - - In addition, an optional positional argument can be used - to specify a device name or a sys path. It must start with - /dev or /sys - respectively. udevadm trigger <arg choice="opt"><replaceable>options</replaceable></arg> - <arg choice="opt" rep="repeat"><replaceable>devpath</replaceable>|<replaceable>file</replaceable></arg> + devpath|file|unit + Request device events from the kernel. Primarily used to replay events at system coldplug time. + + Takes one or more device specifications as arguments. See the description of info + above. + diff --git a/src/udev/udevadm-info.c b/src/udev/udevadm-info.c index ff4cf4ec457..31a6291279b 100644 --- a/src/udev/udevadm-info.c +++ b/src/udev/udevadm-info.c @@ -440,7 +440,7 @@ int info_main(int argc, char *argv[], void *userdata) { r = find_device(argv[optind], NULL, &device); if (r == -EINVAL) - return log_error_errno(r, "Bad argument \"%s\", an absolute path in /dev/ or /sys expected: %m", + return log_error_errno(r, "Bad argument \"%s\", expected an absolute path in /dev/ or /sys or a unit name: %m", argv[optind]); if (r < 0) return log_error_errno(r, "Unknown device \"%s\": %m", argv[optind]); diff --git a/src/udev/udevadm-util.c b/src/udev/udevadm-util.c index ef0dc564ce6..557982b23df 100644 --- a/src/udev/udevadm-util.c +++ b/src/udev/udevadm-util.c @@ -6,18 +6,31 @@ #include "device-private.h" #include "path-util.h" #include "udevadm-util.h" +#include "unit-name.h" int find_device(const char *id, const char *prefix, sd_device **ret) { - _cleanup_free_ char *buf = NULL; + _cleanup_free_ char *path = NULL; + int r; assert(id); assert(ret); - if (prefix && !path_startswith(id, prefix)) { - buf = path_join(prefix, id); - if (!buf) - return -ENOMEM; - id = buf; + if (prefix) { + if (!path_startswith(id, prefix)) { + id = path = path_join(prefix, id); + if (!path) + return -ENOMEM; + } + } else { + /* In cases where the argument is generic (no prefix specified), + * check if the argument looks like a device unit name. */ + if (unit_name_is_valid(id, UNIT_NAME_PLAIN) && + unit_name_to_type(id) == UNIT_DEVICE) { + r = unit_name_to_path(id, &path); + if (r < 0) + return log_debug_errno(r, "Failed to convert \"%s\" to a device path: %m", id); + id = path; + } } if (path_startswith(id, "/sys/")) From 3c79311a6a4518acf68215f72c3e8c78f220e710 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 10 Dec 2018 14:02:39 +0100 Subject: [PATCH 4/4] udevadm: allow multiple arguments to "info" This matches udevadm trigger, which allows multiple arguments since 80877656a55. --- man/udevadm.xml | 8 ++-- src/udev/udevadm-info.c | 87 ++++++++++++++++++++++------------------- 2 files changed, 50 insertions(+), 45 deletions(-) diff --git a/man/udevadm.xml b/man/udevadm.xml index ac3198b1f87..44be7b3f894 100644 --- a/man/udevadm.xml +++ b/man/udevadm.xml @@ -76,14 +76,14 @@ udevadm info <arg choice="opt"><replaceable>options</replaceable></arg> - <arg choice="opt"><replaceable>devpath</replaceable>|<replaceable>file</replaceable>|<replaceable>unit</replaceable></arg> + <arg choice="opt" rep="repeat"><replaceable>devpath</replaceable>|<replaceable>file</replaceable>|<replaceable>unit</replaceable></arg> Query the udev database for device information. - A positional argument should be used to specify a device. It may be a device name (in which case - it must start with /dev/), a sys path (in which case it must start with - /sys/), or a systemd device unit name (in which case it must end with + Positional arguments should be used to specify one or more devices. Each one may be a device name + (in which case it must start with /dev/), a sys path (in which case it must start + with /sys/), or a systemd device unit name (in which case it must end with .device, see systemd.device5). diff --git a/src/udev/udevadm-info.c b/src/udev/udevadm-info.c index 31a6291279b..d141bc74b2a 100644 --- a/src/udev/udevadm-info.c +++ b/src/udev/udevadm-info.c @@ -335,7 +335,7 @@ static int help(void) { } int info_main(int argc, char *argv[], void *userdata) { - _cleanup_(sd_device_unrefp) sd_device *device = NULL; + _cleanup_strv_free_ char **devices = NULL; _cleanup_free_ char *name = NULL; int c, r; @@ -361,25 +361,20 @@ int info_main(int argc, char *argv[], void *userdata) { while ((c = getopt_long(argc, argv, "aced:n:p:q:rxP:RVh", options, NULL)) >= 0) switch (c) { case 'n': - if (device) { - log_error("device already specified"); - return -EINVAL; - } + case 'p': { + const char *prefix = c == 'n' ? "/dev/" : "/sys/"; + char *path; - r = find_device(optarg, "/dev/", &device); - if (r < 0) - return log_error_errno(r, "device node not found: %m"); - break; - case 'p': - if (device) { - log_error("device already specified"); - return -EINVAL; - } + path = path_join(path_startswith(optarg, prefix) ? NULL : prefix, optarg); + if (!path) + return log_oom(); - r = find_device(optarg, "/sys", &device); + r = strv_consume(&devices, path); if (r < 0) - return log_error_errno(r, "syspath not found: %m"); + return log_oom(); break; + } + case 'q': action = ACTION_QUERY; if (streq(optarg, "property") || streq(optarg, "env")) @@ -430,35 +425,45 @@ int info_main(int argc, char *argv[], void *userdata) { assert_not_reached("Unknown option"); } - if (IN_SET(action, ACTION_QUERY, ACTION_ATTRIBUTE_WALK) && - !device) { - /* A device argument is required. It may be an option or a positional arg. */ - if (!argv[optind]) + if (action == ACTION_DEVICE_ID_FILE) { + if (argv[optind]) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "A device name or path is required"); - - r = find_device(argv[optind], NULL, &device); - if (r == -EINVAL) - return log_error_errno(r, "Bad argument \"%s\", expected an absolute path in /dev/ or /sys or a unit name: %m", - argv[optind]); - if (r < 0) - return log_error_errno(r, "Unknown device \"%s\": %m", argv[optind]); - } - - switch (action) { - case ACTION_QUERY: - assert(device); - return query_device(query, device); - - case ACTION_ATTRIBUTE_WALK: - assert(device); - return print_device_chain(device); - - case ACTION_DEVICE_ID_FILE: + "Positional arguments are not allowed with -d/--device-id-of-file."); assert(name); return stat_device(name, arg_export, arg_export_prefix); } - assert_not_reached("Unknown action"); + r = strv_extend_strv(&devices, argv + optind, false); + if (r < 0) + return log_error_errno(r, "Failed to build argument list: %m"); + + if (strv_isempty(devices)) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "A device name or path is required"); + if (action == ACTION_ATTRIBUTE_WALK && strv_length(devices) > 1) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Only one device may be specified with -a/--attribute-walk"); + + char **p; + STRV_FOREACH(p, devices) { + _cleanup_(sd_device_unrefp) sd_device *device = NULL; + + r = find_device(*p, NULL, &device); + if (r == -EINVAL) + return log_error_errno(r, "Bad argument \"%s\", expected an absolute path in /dev/ or /sys or a unit name: %m", *p); + if (r < 0) + return log_error_errno(r, "Unknown device \"%s\": %m", *p); + + if (action == ACTION_QUERY) + r = query_device(query, device); + else if (action == ACTION_ATTRIBUTE_WALK) + r = print_device_chain(device); + else + assert_not_reached("Unknown action"); + if (r < 0) + return r; + } + + return 0; }