From e4080a6d975bf595c64de4c6f59a3ea7e7d8f68b Mon Sep 17 00:00:00 2001 From: David Tardon Date: Fri, 3 Nov 2023 13:12:42 +0100 Subject: [PATCH 1/3] udevadm-control: parse all args in advance This is mostly just refactoring to make the code more manageable and extendable. It doesn't introduce any semantic changes, but it "optimizes" the communication with udevd a bit: * If an option is used multiple times, only one command will be sent. If the option takes a value, the value of the final appearance of the option is used. The only exception is --property, which is additive by nature. * Options --start-exec-queue and --stop-exec-queue are handled together. Only one of START_QUEUE/STOP_QUEUE commands will be sent to udevd even if both options appear on the command line. --- src/udev/udevadm-control.c | 195 +++++++++++++++++++++++++------------ 1 file changed, 132 insertions(+), 63 deletions(-) diff --git a/src/udev/udevadm-control.c b/src/udev/udevadm-control.c index 45a71b23f1..68e3249394 100644 --- a/src/udev/udevadm-control.c +++ b/src/udev/udevadm-control.c @@ -21,12 +21,25 @@ #include "parse-util.h" #include "process-util.h" +#include "static-destruct.h" +#include "strv.h" #include "syslog-util.h" #include "time-util.h" #include "udevadm.h" #include "udev-ctrl.h" #include "virt.h" +static char **arg_env = NULL; +static usec_t arg_timeout = 60 * USEC_PER_SEC; +static bool arg_ping = false; +static bool arg_reload = false; +static bool arg_exit = false; +static int arg_max_children = -1; +static int arg_log_level = -1; +static int arg_start_exec_queue = -1; + +STATIC_DESTRUCTOR_REGISTER(arg_env, strv_freep); + static int help(void) { printf("%s control OPTION\n\n" "Control the udev daemon.\n\n" @@ -46,11 +59,7 @@ static int help(void) { return 0; } -int control_main(int argc, char *argv[], void *userdata) { - _cleanup_(udev_ctrl_unrefp) UdevCtrl *uctrl = NULL; - usec_t timeout = 60 * USEC_PER_SEC; - int c, r; - +static int parse_argv(int argc, char *argv[]) { enum { ARG_PING = 0x100, }; @@ -73,102 +82,78 @@ int control_main(int argc, char *argv[], void *userdata) { {} }; - if (running_in_chroot() > 0) { - log_info("Running in chroot, ignoring request."); - return 0; - } + int c, r; + + assert(argc >= 0); + assert(argv); if (argc <= 1) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "This command expects one or more options."); - r = udev_ctrl_new(&uctrl); - if (r < 0) - return log_error_errno(r, "Failed to initialize udev control: %m"); - while ((c = getopt_long(argc, argv, "el:sSRp:m:t:Vh", options, NULL)) >= 0) switch (c) { - case 'e': - r = udev_ctrl_send_exit(uctrl); - if (r == -ENOANO) - log_warning("Cannot specify --exit after --exit, ignoring."); - else if (r < 0) - return log_error_errno(r, "Failed to send exit request: %m"); - break; - case 'l': - r = log_level_from_string(optarg); - if (r < 0) - return log_error_errno(r, "Failed to parse log level '%s': %m", optarg); - r = udev_ctrl_send_set_log_level(uctrl, r); - if (r == -ENOANO) - log_warning("Cannot specify --log-level after --exit, ignoring."); - else if (r < 0) - return log_error_errno(r, "Failed to send request to set log level: %m"); + case 'e': + arg_exit = true; break; + + case 'l': + arg_log_level = log_level_from_string(optarg); + if (arg_log_level < 0) + return log_error_errno(arg_log_level, "Failed to parse log level '%s': %m", optarg); + break; + case 's': - r = udev_ctrl_send_stop_exec_queue(uctrl); - if (r == -ENOANO) - log_warning("Cannot specify --stop-exec-queue after --exit, ignoring."); - else if (r < 0) - return log_error_errno(r, "Failed to send request to stop exec queue: %m"); + arg_start_exec_queue = false; break; + case 'S': - r = udev_ctrl_send_start_exec_queue(uctrl); - if (r == -ENOANO) - log_warning("Cannot specify --start-exec-queue after --exit, ignoring."); - else if (r < 0) - return log_error_errno(r, "Failed to send request to start exec queue: %m"); + arg_start_exec_queue = true; break; + case 'R': - r = udev_ctrl_send_reload(uctrl); - if (r == -ENOANO) - log_warning("Cannot specify --reload after --exit, ignoring."); - else if (r < 0) - return log_error_errno(r, "Failed to send reload request: %m"); + arg_reload = true; break; + case 'p': if (!strchr(optarg, '=')) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "expect = instead of '%s'", optarg); - r = udev_ctrl_send_set_env(uctrl, optarg); - if (r == -ENOANO) - log_warning("Cannot specify --property after --exit, ignoring."); - else if (r < 0) - return log_error_errno(r, "Failed to send request to update environment: %m"); + r = strv_extend(&arg_env, optarg); + if (r < 0) + return log_error_errno(r, "Failed to extend environment: %m"); + break; + case 'm': { unsigned i; - r = safe_atou(optarg, &i); if (r < 0) return log_error_errno(r, "Failed to parse maximum number of children '%s': %m", optarg); - - r = udev_ctrl_send_set_children_max(uctrl, i); - if (r == -ENOANO) - log_warning("Cannot specify --children-max after --exit, ignoring."); - else if (r < 0) - return log_error_errno(r, "Failed to send request to set number of children: %m"); + arg_max_children = i; break; } + case ARG_PING: - r = udev_ctrl_send_ping(uctrl); - if (r == -ENOANO) - log_error("Cannot specify --ping after --exit, ignoring."); - else if (r < 0) - return log_error_errno(r, "Failed to send a ping message: %m"); + arg_ping = true; break; + case 't': - r = parse_sec(optarg, &timeout); + r = parse_sec(optarg, &arg_timeout); if (r < 0) return log_error_errno(r, "Failed to parse timeout value '%s': %m", optarg); break; + case 'V': return print_version(); + case 'h': return help(); + case '?': return -EINVAL; + default: assert_not_reached(); } @@ -177,7 +162,91 @@ int control_main(int argc, char *argv[], void *userdata) { return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Extraneous argument: %s", argv[optind]); - r = udev_ctrl_wait(uctrl, timeout); + return 1; +} + +int control_main(int argc, char *argv[], void *userdata) { + _cleanup_(udev_ctrl_unrefp) UdevCtrl *uctrl = NULL; + int r; + + if (running_in_chroot() > 0) { + log_info("Running in chroot, ignoring request."); + return 0; + } + + r = parse_argv(argc, argv); + if (r <= 0) + return r; + + r = udev_ctrl_new(&uctrl); + if (r < 0) + return log_error_errno(r, "Failed to initialize udev control: %m"); + + if (arg_exit) { + r = udev_ctrl_send_exit(uctrl); + if (r == -ENOANO) + log_warning("Cannot specify --exit after --exit, ignoring."); + else if (r < 0) + return log_error_errno(r, "Failed to send exit request: %m"); + } + + if (arg_log_level >= 0) { + r = udev_ctrl_send_set_log_level(uctrl, r); + if (r == -ENOANO) + log_warning("Cannot specify --log-level after --exit, ignoring."); + else if (r < 0) + return log_error_errno(r, "Failed to send request to set log level: %m"); + } + + if (arg_start_exec_queue == false) { + r = udev_ctrl_send_stop_exec_queue(uctrl); + if (r == -ENOANO) + log_warning("Cannot specify --stop-exec-queue after --exit, ignoring."); + else if (r < 0) + return log_error_errno(r, "Failed to send request to stop exec queue: %m"); + } + + if (arg_start_exec_queue == true) { + r = udev_ctrl_send_start_exec_queue(uctrl); + if (r == -ENOANO) + log_warning("Cannot specify --start-exec-queue after --exit, ignoring."); + else if (r < 0) + return log_error_errno(r, "Failed to send request to start exec queue: %m"); + } + + if (arg_reload) { + r = udev_ctrl_send_reload(uctrl); + if (r == -ENOANO) + log_warning("Cannot specify --reload after --exit, ignoring."); + else if (r < 0) + return log_error_errno(r, "Failed to send reload request: %m"); + } + + STRV_FOREACH(env, arg_env) { + r = udev_ctrl_send_set_env(uctrl, *env); + if (r == -ENOANO) + log_warning("Cannot specify --property after --exit, ignoring."); + else if (r < 0) + return log_error_errno(r, "Failed to send request to update environment: %m"); + } + + if (arg_max_children) { + r = udev_ctrl_send_set_children_max(uctrl, arg_max_children); + if (r == -ENOANO) + log_warning("Cannot specify --children-max after --exit, ignoring."); + else if (r < 0) + return log_error_errno(r, "Failed to send request to set number of children: %m"); + } + + if (arg_ping) { + r = udev_ctrl_send_ping(uctrl); + if (r == -ENOANO) + log_error("Cannot specify --ping after --exit, ignoring."); + else if (r < 0) + return log_error_errno(r, "Failed to send a ping message: %m"); + } + + r = udev_ctrl_wait(uctrl, arg_timeout); if (r < 0) return log_error_errno(r, "Failed to wait for daemon to reply: %m"); From 3bce9d56fd53fbcd6438c2bf219318133795cb56 Mon Sep 17 00:00:00 2001 From: David Tardon Date: Fri, 3 Nov 2023 14:01:54 +0100 Subject: [PATCH 2/3] udevadm-control: return early if --exit is used All the following commands would just fail with -ENOANO anyway, hence there's no point in sending them. And it allows us to shave off some error handling code. --- src/udev/udevadm-control.c | 35 ++++++++++------------------------- 1 file changed, 10 insertions(+), 25 deletions(-) diff --git a/src/udev/udevadm-control.c b/src/udev/udevadm-control.c index 68e3249394..90b9b39e22 100644 --- a/src/udev/udevadm-control.c +++ b/src/udev/udevadm-control.c @@ -184,65 +184,50 @@ int control_main(int argc, char *argv[], void *userdata) { if (arg_exit) { r = udev_ctrl_send_exit(uctrl); - if (r == -ENOANO) - log_warning("Cannot specify --exit after --exit, ignoring."); - else if (r < 0) - return log_error_errno(r, "Failed to send exit request: %m"); + if (r < 0) + return log_error_errno(r, "Failed to send exit request: %m"); + return 0; } if (arg_log_level >= 0) { r = udev_ctrl_send_set_log_level(uctrl, r); - if (r == -ENOANO) - log_warning("Cannot specify --log-level after --exit, ignoring."); - else if (r < 0) + if (r < 0) return log_error_errno(r, "Failed to send request to set log level: %m"); } if (arg_start_exec_queue == false) { r = udev_ctrl_send_stop_exec_queue(uctrl); - if (r == -ENOANO) - log_warning("Cannot specify --stop-exec-queue after --exit, ignoring."); - else if (r < 0) + if (r < 0) return log_error_errno(r, "Failed to send request to stop exec queue: %m"); } if (arg_start_exec_queue == true) { r = udev_ctrl_send_start_exec_queue(uctrl); - if (r == -ENOANO) - log_warning("Cannot specify --start-exec-queue after --exit, ignoring."); - else if (r < 0) + if (r < 0) return log_error_errno(r, "Failed to send request to start exec queue: %m"); } if (arg_reload) { r = udev_ctrl_send_reload(uctrl); - if (r == -ENOANO) - log_warning("Cannot specify --reload after --exit, ignoring."); - else if (r < 0) + if (r < 0) return log_error_errno(r, "Failed to send reload request: %m"); } STRV_FOREACH(env, arg_env) { r = udev_ctrl_send_set_env(uctrl, *env); - if (r == -ENOANO) - log_warning("Cannot specify --property after --exit, ignoring."); - else if (r < 0) + if (r < 0) return log_error_errno(r, "Failed to send request to update environment: %m"); } if (arg_max_children) { r = udev_ctrl_send_set_children_max(uctrl, arg_max_children); - if (r == -ENOANO) - log_warning("Cannot specify --children-max after --exit, ignoring."); - else if (r < 0) + if (r < 0) return log_error_errno(r, "Failed to send request to set number of children: %m"); } if (arg_ping) { r = udev_ctrl_send_ping(uctrl); - if (r == -ENOANO) - log_error("Cannot specify --ping after --exit, ignoring."); - else if (r < 0) + if (r < 0) return log_error_errno(r, "Failed to send a ping message: %m"); } From a859f794f96791b9ae2938b24592a140c764143d Mon Sep 17 00:00:00 2001 From: David Tardon Date: Fri, 3 Nov 2023 14:08:37 +0100 Subject: [PATCH 3/3] udev-ctrl: drop the unused maybe_disconnected stuff --- src/udev/udev-ctrl.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/udev/udev-ctrl.c b/src/udev/udev-ctrl.c index 24ba8231f5..28716347e7 100644 --- a/src/udev/udev-ctrl.c +++ b/src/udev/udev-ctrl.c @@ -37,7 +37,6 @@ struct UdevCtrl { socklen_t addrlen; bool bound; bool connected; - bool maybe_disconnected; sd_event *event; sd_event_source *event_source; sd_event_source *event_source_connect; @@ -291,9 +290,6 @@ int udev_ctrl_send(UdevCtrl *uctrl, UdevCtrlMessageType type, const void *data) .type = type, }; - if (uctrl->maybe_disconnected) - return -ENOANO; /* to distinguish this from other errors. */ - if (type == UDEV_CTRL_SET_ENV) { assert(data); strscpy(ctrl_msg_wire.value.buf, sizeof(ctrl_msg_wire.value.buf), data); @@ -309,9 +305,6 @@ int udev_ctrl_send(UdevCtrl *uctrl, UdevCtrlMessageType type, const void *data) if (send(uctrl->sock, &ctrl_msg_wire, sizeof(ctrl_msg_wire), 0) < 0) return -errno; - if (type == UDEV_CTRL_EXIT) - uctrl->maybe_disconnected = true; - return 0; } @@ -326,11 +319,9 @@ int udev_ctrl_wait(UdevCtrl *uctrl, usec_t timeout) { if (!uctrl->connected) return 0; - if (!uctrl->maybe_disconnected) { - r = udev_ctrl_send(uctrl, _UDEV_CTRL_END_MESSAGES, NULL); - if (r < 0) - return r; - } + r = udev_ctrl_send(uctrl, _UDEV_CTRL_END_MESSAGES, NULL); + if (r < 0) + return r; if (timeout == 0) return 0;