From 2f42b19c0f7b553d343b69691a5e0f1225f5a47d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 25 Apr 2026 11:58:50 +0200 Subject: [PATCH 1/9] v4l_id: convert to the new option parser The commandline check is tightened to reject extra arguments. Co-developed-by: Claude Opus 4.7 --- src/udev/v4l_id/v4l_id.c | 59 ++++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 24 deletions(-) diff --git a/src/udev/v4l_id/v4l_id.c b/src/udev/v4l_id/v4l_id.c index dc4d41af2b..c5bfa935b2 100644 --- a/src/udev/v4l_id/v4l_id.c +++ b/src/udev/v4l_id/v4l_id.c @@ -4,7 +4,6 @@ */ #include -#include #include #include #include @@ -12,43 +11,55 @@ #include "build.h" #include "errno-util.h" #include "fd-util.h" +#include "format-table.h" +#include "help-util.h" #include "log.h" #include "main-func.h" +#include "options.h" #include "string-util.h" +#include "strv.h" #include "utf8.h" static const char *arg_device = NULL; -static int parse_argv(int argc, char *argv[]) { - static const struct option options[] = { - { "help", no_argument, NULL, 'h' }, - { "version", no_argument, NULL, 'v' }, - {} - }; - int c; +static int help(void) { + _cleanup_(table_unrefp) Table *options = NULL; + int r; - while ((c = getopt_long(argc, argv, "h", options, NULL)) >= 0) + r = option_parser_get_help_table(&options); + if (r < 0) + return r; + + help_cmdline("[OPTIONS...] DEVICE"); + help_abstract("Video4Linux device identification."); + help_section("Options:"); + + return table_print_or_warn(options); +} + +static int parse_argv(int argc, char *argv[]) { + assert(argc >= 0); + assert(argv); + + OptionParser state = { argc, argv }; + const char *arg; + + FOREACH_OPTION(&state, c, &arg, /* on_error= */ return c) switch (c) { - case 'h': - printf("%s [OPTIONS...] DEVICE\n\n" - "Video4Linux device identification.\n\n" - " -h --help Show this help text\n" - " --version Show package version\n", - program_invocation_short_name); - return 0; - case 'v': + + OPTION_COMMON_HELP: + return help(); + + OPTION_COMMON_VERSION: return version(); - case '?': - return -EINVAL; - default: - assert_not_reached(); } - if (!argv[optind]) + char **args = option_parser_get_args(&state); + if (strv_length(args) != 1) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "DEVICE argument missing."); + "Need exactly one DEVICE argument."); - arg_device = argv[optind]; + arg_device = args[0]; return 1; } From dad6381d45ad2e57d73a0fe7ed99c3635a3cbaf8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 25 Apr 2026 12:05:47 +0200 Subject: [PATCH 2/9] mtd_probe: convert to the new option parser The commandline check is tightened. Previously the program would crash if called without an argument. Co-developed-by: Claude Opus 4.7 --- src/udev/mtd_probe/mtd_probe.c | 63 ++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/src/udev/mtd_probe/mtd_probe.c b/src/udev/mtd_probe/mtd_probe.c index d79a0617e2..1cc769ad76 100644 --- a/src/udev/mtd_probe/mtd_probe.c +++ b/src/udev/mtd_probe/mtd_probe.c @@ -19,48 +19,59 @@ */ #include -#include #include -#include #include #include "build.h" #include "errno-util.h" #include "fd-util.h" +#include "format-table.h" +#include "help-util.h" #include "log.h" #include "main-func.h" #include "mtd_probe.h" +#include "options.h" +#include "strv.h" static const char *arg_device = NULL; -static int parse_argv(int argc, char *argv[]) { - static const struct option options[] = { - { "help", no_argument, NULL, 'h' }, - { "version", no_argument, NULL, 'v' }, - {} - }; - int c; +static int help(void) { + _cleanup_(table_unrefp) Table *options = NULL; + int r; - while ((c = getopt_long(argc, argv, "h", options, NULL)) >= 0) + r = option_parser_get_help_table(&options); + if (r < 0) + return r; + + help_cmdline("[OPTIONS...] /dev/mtd[n]"); + help_abstract("Probe MTD devices."); + help_section("Options:"); + + return table_print_or_warn(options); +} + +static int parse_argv(int argc, char *argv[]) { + assert(argc >= 0); + assert(argv); + + OptionParser state = { argc, argv }; + const char *arg; + + FOREACH_OPTION(&state, c, &arg, /* on_error= */ return c) switch (c) { - case 'h': - printf("%s /dev/mtd[n]\n\n" - " -h --help Show this help text\n" - " --version Show package version\n", - program_invocation_short_name); - return 0; - case 'v': + + OPTION_COMMON_HELP: + return help(); + + OPTION_COMMON_VERSION: return version(); - case '?': - return -EINVAL; - default: - assert_not_reached(); } - if (argc > 2) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Error: unexpected argument."); + char **args = option_parser_get_args(&state); + if (strv_length(args) != 1) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Need exactly one DEVICE argument."); - arg_device = argv[optind]; + arg_device = args[0]; return 1; } @@ -73,12 +84,12 @@ static int run(int argc, char** argv) { if (r <= 0) return r; - mtd_fd = open(argv[1], O_RDONLY|O_CLOEXEC|O_NOCTTY); + mtd_fd = open(arg_device, O_RDONLY|O_CLOEXEC|O_NOCTTY); if (mtd_fd < 0) { bool ignore = ERRNO_IS_DEVICE_ABSENT_OR_EMPTY(errno); log_full_errno(ignore ? LOG_DEBUG : LOG_WARNING, errno, "Failed to open device node '%s'%s: %m", - argv[1], ignore ? ", ignoring" : ""); + arg_device, ignore ? ", ignoring" : ""); return ignore ? 0 : -errno; } From e8e75a1c99bfade54f9af90cc039ea897e839595 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 25 Apr 2026 12:09:05 +0200 Subject: [PATCH 3/9] fido_id: convert to the new option parser The commandline check is tightened. Previously the program would crash if called without an argument. Co-developed-by: Claude Opus 4.7 --- src/udev/fido_id/fido_id.c | 56 +++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/src/udev/fido_id/fido_id.c b/src/udev/fido_id/fido_id.c index aa918a9798..a301b9acdb 100644 --- a/src/udev/fido_id/fido_id.c +++ b/src/udev/fido_id/fido_id.c @@ -7,7 +7,6 @@ */ #include -#include #include #include #include @@ -18,41 +17,54 @@ #include "device-util.h" #include "fd-util.h" #include "fido_id_desc.h" +#include "format-table.h" +#include "help-util.h" #include "log.h" #include "main-func.h" +#include "options.h" #include "path-util.h" +#include "strv.h" #include "udev-util.h" static const char *arg_device = NULL; -static int parse_argv(int argc, char *argv[]) { - static const struct option options[] = { - { "help", no_argument, NULL, 'h' }, - { "version", no_argument, NULL, 'v' }, - {} - }; - int c; +static int help(void) { + _cleanup_(table_unrefp) Table *options = NULL; + int r; - while ((c = getopt_long(argc, argv, "h", options, NULL)) >= 0) + r = option_parser_get_help_table(&options); + if (r < 0) + return r; + + help_cmdline("[OPTIONS...] SYSFS_PATH"); + help_abstract("Identify FIDO security tokens."); + help_section("Options:"); + + return table_print_or_warn(options); +} + +static int parse_argv(int argc, char *argv[]) { + assert(argc >= 0); + assert(argv); + + OptionParser state = { argc, argv }; + const char *arg; + + FOREACH_OPTION(&state, c, &arg, /* on_error= */ return c) switch (c) { - case 'h': - printf("%s [OPTIONS...] SYSFS_PATH\n\n" - " -h --help Show this help text\n" - " --version Show package version\n", - program_invocation_short_name); - return 0; - case 'v': + + OPTION_COMMON_HELP: + return help(); + + OPTION_COMMON_VERSION: return version(); - case '?': - return -EINVAL; - default: - assert_not_reached(); } - if (argc > 2) + char **args = option_parser_get_args(&state); + if (strv_length(args) > 1) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Error: unexpected argument."); - arg_device = argv[optind]; + arg_device = args[0]; return 1; } From 9286f1f2997d2d547cef7574d612e22285e10374 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 25 Apr 2026 12:17:27 +0200 Subject: [PATCH 4/9] dmi_memory_id: convert to the new option parser For some reason, the old code had duplicate {"version", 'v'} entry in the options table and a matching case that could not be reached. Commandline parsing is tightened to reject any positional arguments. Co-developed-by: Claude Opus 4.7 --- src/shared/options.h | 6 +++ src/udev/dmi_memory_id/dmi_memory_id.c | 64 ++++++++++++++------------ 2 files changed, 41 insertions(+), 29 deletions(-) diff --git a/src/shared/options.h b/src/shared/options.h index d86766f90e..f0c19af49c 100644 --- a/src/shared/options.h +++ b/src/shared/options.h @@ -109,6 +109,12 @@ typedef struct Option { "Allows the certificate to be loaded from an OpenSSL provider " \ "(file, provider:PROVIDER)") +/* A form used in udev code for compatibility. -V is accepted but not documented. */ +#define OPTION_COMMON_VERSION_WITH_HIDDEN_V \ + OPTION_COMMON_VERSION: {} \ + OPTION_SHORT('V', NULL, /* help= */ NULL) + + /* This is magically mapped to the beginning and end of the section */ extern const Option __start_SYSTEMD_OPTIONS[]; extern const Option __stop_SYSTEMD_OPTIONS[]; diff --git a/src/udev/dmi_memory_id/dmi_memory_id.c b/src/udev/dmi_memory_id/dmi_memory_id.c index a2f2ed7263..df683d0072 100644 --- a/src/udev/dmi_memory_id/dmi_memory_id.c +++ b/src/udev/dmi_memory_id/dmi_memory_id.c @@ -42,13 +42,15 @@ * https://www.dmtf.org/sites/default/files/DSP0270_1.0.1.pdf */ -#include #include #include "alloc-util.h" #include "build.h" #include "fileio.h" +#include "format-table.h" +#include "help-util.h" #include "main-func.h" +#include "options.h" #include "string-util.h" #include "udev-util.h" #include "unaligned.h" @@ -643,45 +645,49 @@ static int legacy_decode(const uint8_t *buf, const char *devmem, bool no_file_of } static int help(void) { - printf("%s [OPTIONS...]\n\n" - " -F --from-dump FILE Read DMI information from a binary file\n" - " -h --help Show this help text\n" - " --version Show package version\n", - program_invocation_short_name); - return 0; + _cleanup_(table_unrefp) Table *options = NULL; + int r; + + r = option_parser_get_help_table(&options); + if (r < 0) + return r; + + help_cmdline("[OPTIONS...]"); + help_section("Options:"); + + return table_print_or_warn(options); } -static int parse_argv(int argc, char * const *argv) { - static const struct option options[] = { - { "from-dump", required_argument, NULL, 'F' }, - { "version", no_argument, NULL, 'V' }, - { "help", no_argument, NULL, 'h' }, - { "version", no_argument, NULL, 'v' }, - {} - }; - int c; +static int parse_argv(int argc, char *argv[]) { + assert(argc >= 0); + assert(argv); - while ((c = getopt_long(argc, argv, "F:hV", options, NULL)) >= 0) + OptionParser state = { argc, argv }; + const char *arg; + + FOREACH_OPTION(&state, c, &arg, /* on_error= */ return c) switch (c) { - case 'F': - arg_source_file = optarg; - break; - case 'V': - return version(); - case 'h': + + OPTION_COMMON_HELP: return help(); - case '?': - return -EINVAL; - case 'v': + + OPTION_COMMON_VERSION_WITH_HIDDEN_V: return version(); - default: - assert_not_reached(); + + OPTION('F', "from-dump", "FILE", + "Read DMI information from a binary file"): + arg_source_file = arg; + break; } + if (option_parser_get_n_args(&state) > 0) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "This program takes no arguments."); + return 1; } -static int run(int argc, char* const* argv) { +static int run(int argc, char *argv[]) { _cleanup_free_ uint8_t *buf = NULL; bool no_file_offset = false; size_t size; From 3ad76abc43585109453449f9660aa34f4bb1d028 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 25 Apr 2026 12:33:03 +0200 Subject: [PATCH 5/9] cdrom_id: convert to the new option parser The commandline check is tightened to reject extra arguments. Co-developed-by: Claude Opus 4.7 --- src/udev/cdrom_id/cdrom_id.c | 78 +++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 36 deletions(-) diff --git a/src/udev/cdrom_id/cdrom_id.c b/src/udev/cdrom_id/cdrom_id.c index d311f2b322..1a167475d2 100644 --- a/src/udev/cdrom_id/cdrom_id.c +++ b/src/udev/cdrom_id/cdrom_id.c @@ -4,7 +4,6 @@ */ #include -#include #include #include #include @@ -13,11 +12,15 @@ #include "build.h" #include "errno-util.h" #include "fd-util.h" +#include "format-table.h" +#include "help-util.h" #include "main-func.h" +#include "options.h" #include "random-util.h" #include "sort-util.h" #include "string-table.h" #include "string-util.h" +#include "strv.h" #include "time-util.h" #include "udev-util.h" #include "unaligned.h" @@ -898,60 +901,63 @@ static void print_properties(const Context *c) { } static int help(void) { - printf("%s [OPTIONS...] DEVICE\n\n" - " -l --lock-media Lock the media (to enable eject request events)\n" - " -u --unlock-media Unlock the media\n" - " -e --eject-media Eject the media\n" - " -d --debug Print debug messages to stderr\n" - " -h --help Show this help text\n" - " --version Show package version\n", - program_invocation_short_name); + _cleanup_(table_unrefp) Table *options = NULL; + int r; - return 0; + r = option_parser_get_help_table(&options); + if (r < 0) + return r; + + help_cmdline("[OPTIONS...] DEVICE"); + help_section("Options:"); + + return table_print_or_warn(options); } static int parse_argv(int argc, char *argv[]) { - static const struct option options[] = { - { "lock-media", no_argument, NULL, 'l' }, - { "unlock-media", no_argument, NULL, 'u' }, - { "eject-media", no_argument, NULL, 'e' }, - { "debug", no_argument, NULL, 'd' }, - { "help", no_argument, NULL, 'h' }, - { "version", no_argument, NULL, 'v' }, - {} - }; - int c; + assert(argc >= 0); + assert(argv); - while ((c = getopt_long(argc, argv, "deluh", options, NULL)) >= 0) + OptionParser state = { argc, argv }; + const char *arg; + + FOREACH_OPTION(&state, c, &arg, /* on_error= */ return c) switch (c) { - case 'l': + + OPTION_COMMON_HELP: + return help(); + + OPTION_COMMON_VERSION: + return version(); + + OPTION('l', "lock-media", NULL, + "Lock the media (to enable eject request events)"): arg_lock = true; break; - case 'u': + + OPTION('u', "unlock-media", NULL, + "Unlock the media"): arg_unlock = true; break; - case 'e': + + OPTION('e', "eject-media", NULL, + "Eject the media"): arg_eject = true; break; - case 'd': + + OPTION('d', "debug", NULL, + "Print debug messages to stderr"): log_set_target(LOG_TARGET_CONSOLE); log_set_max_level(LOG_DEBUG); log_open(); break; - case 'h': - return help(); - case 'v': - return version(); - case '?': - return -EINVAL; - default: - assert_not_reached(); } - arg_node = argv[optind]; - if (!arg_node) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "No device specified."); + char **args = option_parser_get_args(&state); + if (strv_length(args) != 1) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Need exactly one DEVICE argument."); + arg_node = args[0]; return 1; } From beea13b9d10ea241931dc1815228247e9b0d62d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 25 Apr 2026 12:43:12 +0200 Subject: [PATCH 6/9] ata_id: convert to the new option parser The commandline check is tightened to reject extra arguments. Co-developed-by: Claude Opus 4.7 --- src/udev/ata_id/ata_id.c | 66 +++++++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/src/udev/ata_id/ata_id.c b/src/udev/ata_id/ata_id.c index 508f99b01a..112456ffc2 100644 --- a/src/udev/ata_id/ata_id.c +++ b/src/udev/ata_id/ata_id.c @@ -6,7 +6,6 @@ */ #include -#include #include #include #include @@ -18,8 +17,12 @@ #include "device-nodes.h" #include "errno-util.h" #include "fd-util.h" +#include "format-table.h" +#include "help-util.h" #include "log.h" #include "main-func.h" +#include "options.h" +#include "strv.h" #include "udev-util.h" #include "unaligned.h" @@ -356,40 +359,47 @@ static int disk_identify(int fd, return 0; } -static int parse_argv(int argc, char *argv[]) { - static const struct option options[] = { - { "export", no_argument, NULL, 'x' }, - { "help", no_argument, NULL, 'h' }, - { "version", no_argument, NULL, 'v' }, - {} - }; - int c; +static int help(void) { + _cleanup_(table_unrefp) Table *options = NULL; + int r; - while ((c = getopt_long(argc, argv, "xh", options, NULL)) >= 0) + r = option_parser_get_help_table(&options); + if (r < 0) + return r; + + help_cmdline("[OPTIONS...] DEVICE"); + help_section("Options:"); + + return table_print_or_warn(options); +} + +static int parse_argv(int argc, char *argv[]) { + assert(argc >= 0); + assert(argv); + + OptionParser state = { argc, argv }; + const char *arg; + + FOREACH_OPTION(&state, c, &arg, /* on_error= */ return c) switch (c) { - case 'x': + + OPTION_COMMON_HELP: + return help(); + + OPTION_COMMON_VERSION: + return version(); + + OPTION('x', "export", NULL, + "Print values as environment keys"): arg_export = true; break; - case 'h': - printf("%s [OPTIONS...] DEVICE\n\n" - " -x --export Print values as environment keys\n" - " -h --help Show this help text\n" - " --version Show package version\n", - program_invocation_short_name); - return 0; - case 'v': - return version(); - case '?': - return -EINVAL; - default: - assert_not_reached(); } - if (!argv[optind]) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "DEVICE argument missing."); + char **args = option_parser_get_args(&state); + if (strv_length(args) != 1) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Need exactly one DEVICE argument."); - arg_device = argv[optind]; + arg_device = args[0]; return 1; } From 4339197f5d4f712bc900d8e09c892015d48b19bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 25 Apr 2026 13:31:43 +0200 Subject: [PATCH 7/9] shared/options: split out helper to format -o/--option= --- src/shared/options.c | 89 ++++++++++++++++++++++++++--------------- src/shared/options.h | 1 + src/test/test-options.c | 49 +++++++++++++++++++++++ 3 files changed, 107 insertions(+), 32 deletions(-) diff --git a/src/shared/options.c b/src/shared/options.c index fbf9a31181..a4c6514508 100644 --- a/src/shared/options.c +++ b/src/shared/options.c @@ -311,6 +311,59 @@ size_t option_parser_get_n_args(const OptionParser *state) { return state->argc - state->positional_offset; } +char* option_get_synopsis(const char *prefix, const Option *opt, const char *joiner, bool show_metavar) { + assert(opt); + assert(!FLAGS_SET(opt->flags, OPTION_GROUP_MARKER)); /* A group marker should not be displayed */ + + if (!prefix) + prefix = ""; + + if (opt->flags & (OPTION_HELP_ENTRY_VERBATIM | OPTION_POSITIONAL_ENTRY)) + return strjoin(prefix, ASSERT_PTR(opt->long_code)); + + /* The option formatted appropriately for --help strings, error messages, and similar: + * ---=[] + * "=" is shown only when a long form is defined: -l --long=ARG, --long=ARG, -s ARG. + * The joiner arg is used between the short and long forms. + * As a special case, if the option has no long form and show_metavar is true, + * a space is used ('-a ARG' or '-a [ARG]'). + */ + assert(opt->short_code != 0 || opt->long_code); + + char sc[3] = ""; + if (opt->short_code != 0) + xsprintf(sc, "-%c", opt->short_code); + + if (show_metavar && opt->metavar && !opt->long_code) + joiner = " "; /* Return '-x ARG', no matter what joiner was specified. */ + else if (opt->short_code == 0 || !opt->long_code) + joiner = ""; + else if (!joiner) + joiner = " "; + + bool need_eq = option_takes_arg(opt) && opt->long_code; + if (!show_metavar) + return strjoin(prefix, + sc, + joiner, + opt->long_code ? "--" : "", + strempty(opt->long_code), + need_eq ? "=" : ""); + + bool need_quote = opt->metavar && strchr(opt->metavar, ' '); + return strjoin(prefix, + sc, + joiner, + opt->long_code ? "--" : "", + strempty(opt->long_code), + option_arg_optional(opt) ? "[" : "", + need_eq ? "=" : "", + need_quote ? "'" : "", + strempty(opt->metavar), + need_quote ? "'" : "", + option_arg_optional(opt) ? "]" : ""); +} + int _option_parser_get_help_table( const Option options[], const Option options_end[], @@ -341,38 +394,10 @@ int _option_parser_get_help_table( /* No help string — we do not show the option */ continue; - _cleanup_free_ char *s = NULL; - - if (FLAGS_SET(opt->flags, OPTION_HELP_ENTRY_VERBATIM)) { - assert(opt->long_code); - - s = strjoin(" ", - opt->long_code); - } else { - char sc[3] = " "; - if (opt->short_code != 0) - xsprintf(sc, "-%c", opt->short_code); - - /* We indent the option string by two spaces. We could set the minimum cell width and - * right-align for a similar result, but that'd be more work. This is only used for - * display. - * - * "=" is shown only when a long option is defined: -l --long=ARG, --long=ARG, -s ARG. - */ - bool need_eq = option_takes_arg(opt) && opt->long_code; - bool need_quote = opt->metavar && strchr(opt->metavar, ' '); - s = strjoin(" ", - sc, - " ", - opt->long_code ? "--" : "", - strempty(opt->long_code), - option_arg_optional(opt) ? "[" : "", - need_eq ? "=" : "", - need_quote ? "'" : "", - strempty(opt->metavar), - need_quote ? "'" : "", - option_arg_optional(opt) ? "]" : ""); - } + /* We indent the option string by two spaces. We could set the minimum cell width and + * right-align for a similar result, but that'd be more work. This is only used for + * display. */ + _cleanup_free_ char *s = option_get_synopsis(" ", opt, " ", /* show_metavar= */ true); if (!s) return log_oom(); diff --git a/src/shared/options.h b/src/shared/options.h index f0c19af49c..59b20bc047 100644 --- a/src/shared/options.h +++ b/src/shared/options.h @@ -172,6 +172,7 @@ char* option_parser_consume_next_arg(OptionParser *state); char** option_parser_get_args(const OptionParser *state); size_t option_parser_get_n_args(const OptionParser *state); +char* option_get_synopsis(const char *prefix, const Option *opt, const char *joiner, bool show_metavar); int _option_parser_get_help_table( const Option options[], diff --git a/src/test/test-options.c b/src/test/test-options.c index 1c9073b7a5..49e1b3069f 100644 --- a/src/test/test-options.c +++ b/src/test/test-options.c @@ -1302,4 +1302,53 @@ TEST(option_optional_arg_consume) { } } +static void test_option_get_synopsis_one( + const Option *opt, + const char *joiner, + bool show_metavar, + const char *expected) { + log_debug("%s", expected); + _cleanup_free_ char *s = option_get_synopsis(". ", opt, joiner, show_metavar); + ASSERT_STREQ(s, expected); +} + +TEST(option_get_synopsis) { + test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "X" }, "/", true, ". -x/--xxx=X"); + test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "X" }, NULL, true, ". -x --xxx=X"); + test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "X" }, "/", false, ". -x/--xxx=" ); + test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "X" }, " ", true, ". -x --xxx=X"); + test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "X" }, " ", false, ". -x --xxx=" ); + test_option_get_synopsis_one(&(const Option) { 0, 0, 0, "xxx", "X" }, "+", true, ". --xxx=X" ); + test_option_get_synopsis_one(&(const Option) { 0, 0, 0, "xxx", "X" }, "+", false, ". --xxx=" ); + test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', NULL, "X" }, " ", true, ". -x X" ); + test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', NULL, "X" }, "/", false, ". -x" ); + + test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "A B" }, "/", true, ". -x/--xxx='A B'"); + test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', "xxx", "A B" }, " ", true, ". -x --xxx='A B'"); + test_option_get_synopsis_one(&(const Option) { 0, 0, 0, "xxx", "A B" }, "+", true, ". --xxx='A B'" ); + test_option_get_synopsis_one(&(const Option) { 0, 0, 'x', NULL, "A B" }, " ", true, ". -x 'A B'" ); + + test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "X" }, "/", true, ". -x/--xxx[=X]"); + test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "X" }, NULL, true, ". -x --xxx[=X]"); + /* Note: --xxx[=] would be silly, so we show --xxx=. It's a corner case. Maybe this should change. */ + test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "X" }, "/", false, ". -x/--xxx=" ); + test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "X" }, " ", true, ". -x --xxx[=X]"); + test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "X" }, " ", false, ". -x --xxx=" ); + test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 0, "xxx", "X" }, "+", true, ". --xxx[=X]" ); + test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 0, "xxx", "X" }, "+", false, ". --xxx=" ); + test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', NULL, "X" }, " ", true, ". -x [X]" ); + test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', NULL, "X" }, "/", false, ". -x" ); + + test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "A B" }, "/", true, ". -x/--xxx[='A B']"); + test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', "xxx", "A B" }, " ", true, ". -x --xxx[='A B']"); + test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 0, "xxx", "A B" }, "+", true, ". --xxx[='A B']" ); + test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG, 'x', NULL, "A B" }, " ", true, ". -x ['A B']" ); + + test_option_get_synopsis_one(&(const Option) { 0, OPTION_OPTIONAL_ARG | OPTION_HELP_ENTRY | OPTION_STOPS_PARSING, + 'x', "xxx", "A B" }, "/", true, ". -x/--xxx[='A B']"); + + test_option_get_synopsis_one(&(const Option) { 0, OPTION_HELP_ENTRY_VERBATIM, 'u', "special special", "unused" }, "/", true, ". special special"); + test_option_get_synopsis_one(&(const Option) { 0, OPTION_POSITIONAL_ENTRY, 'u', "(fixed)", "unused" }, "/", true, ". (fixed)"); +} + DEFINE_TEST_MAIN(LOG_DEBUG); From 05fea7df1bd6579dc382455626e0e84acb2a8912 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 25 Apr 2026 13:01:13 +0200 Subject: [PATCH 8/9] scsi_id: convert to the new option parser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Common page-code parsing extracted into a parse_page_code() helper. While at it, return real error values (-EINVAL, etc.) rather than -1, and rename retval to r throughout for consistency. The body of main is moved to new run. The closing of logging file descriptors is dropped. They will be closed automatically anyway. Not sure what the original purpose of that code was. The code is also modernized in various places… though more changes could be made. The return convention of help() and similar functions is changed to usual negative/0/1, where 0 means that the caller should quit. set_inq_values would return positive error values too, which was previously ignored. It's not entirely clear, but that doesn't seem to have been on purpose. --- src/udev/scsi_id/scsi_id.c | 381 +++++++++++++++------------------ src/udev/scsi_id/scsi_serial.c | 35 ++- 2 files changed, 181 insertions(+), 235 deletions(-) diff --git a/src/udev/scsi_id/scsi_id.c b/src/udev/scsi_id/scsi_id.c index ba7a7fe5f5..8f580a8cec 100644 --- a/src/udev/scsi_id/scsi_id.c +++ b/src/udev/scsi_id/scsi_id.c @@ -5,7 +5,6 @@ */ #include -#include #include #include @@ -15,6 +14,10 @@ #include "extract-word.h" #include "fd-util.h" #include "fileio.h" +#include "format-table.h" +#include "help-util.h" +#include "main-func.h" +#include "options.h" #include "parse-util.h" #include "scsi_id.h" #include "string-util.h" @@ -23,23 +26,6 @@ #include "udev-util.h" #include "utf8.h" -static const struct option options[] = { - { "device", required_argument, NULL, 'd' }, - { "config", required_argument, NULL, 'f' }, - { "page", required_argument, NULL, 'p' }, - { "denylisted", no_argument, NULL, 'b' }, - { "allowlisted", no_argument, NULL, 'g' }, - { "blacklisted", no_argument, NULL, 'b' }, /* backward compat */ - { "whitelisted", no_argument, NULL, 'g' }, /* backward compat */ - { "replace-whitespace", no_argument, NULL, 'u' }, - { "sg-version", required_argument, NULL, 's' }, - { "verbose", no_argument, NULL, 'v' }, - { "version", no_argument, NULL, 'V' }, /* don't advertise -V */ - { "export", no_argument, NULL, 'x' }, - { "help", no_argument, NULL, 'h' }, - {} -}; - static bool all_good = false; static bool dev_specified = false; static char config_file[MAX_PATH_LEN] = "/etc/scsi_id.config"; @@ -101,31 +87,22 @@ static void set_type(unsigned type_num, char *to, size_t len) { * * vendor and model can end in '\n'. */ -static int get_file_options(const char *vendor, const char *model, - int *argc, char ***newargv) { +static int get_file_options(const char *vendor, const char *model, char ***ret_argv) { _cleanup_free_ char *vendor_in = NULL, *model_in = NULL, *options_in = NULL; /* read in from file */ _cleanup_strv_free_ char **options_argv = NULL; - _cleanup_fclose_ FILE *f = NULL; - int lineno, r; + int r; - f = fopen(config_file, "re"); + _cleanup_fclose_ FILE *f = fopen(config_file, "re"); if (!f) { if (errno == ENOENT) - return 1; - else { - log_error_errno(errno, "can't open %s: %m", config_file); - return -1; - } + goto finish; + return log_error_errno(errno, "Cannot open %s: %m", config_file); } - *newargv = NULL; - lineno = 0; - for (;;) { + for (int lineno = 0;;) { _cleanup_free_ char *buffer = NULL, *key = NULL, *value = NULL; const char *buf; - vendor_in = model_in = options_in = NULL; - r = read_line(f, MAX_BUFFER_LEN, &buffer); if (r < 0) return log_error_errno(r, "read_line() on line %d of %s failed: %m", lineno, config_file); @@ -197,189 +174,179 @@ static int get_file_options(const char *vendor, const char *model, } - if (vendor_in == NULL && model_in == NULL && options_in == NULL) - return 1; /* No matches */ + if (!vendor_in && !model_in && !options_in) + goto finish; /* No matches */ - /* - * Something matched. Allocate newargv, and store - * values found in options_in. - */ + /* Something matched. Allocate newargv, and store values found in options_in. */ options_argv = strv_split(options_in, " \t"); if (!options_argv) return log_oom(); - r = strv_prepend(&options_argv, ""); /* getopt skips over argv[0] */ + r = strv_prepend(&options_argv, ""); /* argv[0] is skipped */ if (r < 0) return r; - *newargv = TAKE_PTR(options_argv); - *argc = strv_length(*newargv); + finish: + *ret_argv = TAKE_PTR(options_argv); + return !!*ret_argv; /* true if something matched, false otherwise */ +} + +static int parse_page_code(const char *value, enum page_code *ret) { + assert(value); + assert(ret); + + if (streq(value, "0x80")) + *ret = PAGE_80; + else if (streq(value, "0x83")) + *ret = PAGE_83; + else if (streq(value, "pre-spc3-83")) + *ret = PAGE_83_PRE_SPC3; + else + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Unknown page code '%s'", value); return 0; } -static void help(void) { - printf("Usage: %s [OPTION...] DEVICE\n\n" - "SCSI device identification.\n\n" - " -h --help Print this message\n" - " --version Print version of the program\n\n" - " -d --device= Device node for SG_IO commands\n" - " -f --config= Location of config file\n" - " -p --page=0x80|0x83|pre-spc3-83 SCSI page (0x80, 0x83, pre-spc3-83)\n" - " -s --sg-version=3|4 Use SGv3 or SGv4\n" - " -b --denylisted Treat device as denylisted\n" - " -g --allowlisted Treat device as allowlisted\n" - " -u --replace-whitespace Replace all whitespace by underscores\n" - " -v --verbose Verbose logging\n" - " -x --export Print values as environment keys\n", - program_invocation_short_name); +static int help(void) { + _cleanup_(table_unrefp) Table *options = NULL; + int r; + + r = option_parser_get_help_table(&options); + if (r < 0) + return r; + + help_cmdline("[OPTION...] DEVICE"); + help_abstract("SCSI device identification."); + help_section("Options:"); + + return table_print_or_warn(options); } -static int set_options(int argc, char **argv, - char *maj_min_dev) { - int option, r; +static int set_options(int argc, char **argv, char *maj_min_dev) { + assert(argc >= 0); + assert(argv); - /* - * optind is a global extern used by getopt. Since we can call - * set_options twice (once for command line, and once for config - * file) we have to reset this back to 1. - */ - optind = 1; - while ((option = getopt_long(argc, argv, "d:f:gp:uvVxhbs:", options, NULL)) >= 0) - switch (option) { - case 'b': + OptionParser state = { argc, argv }; + const char *arg; + int r; + + FOREACH_OPTION(&state, c, &arg, /* on_error= */ return c) + switch (c) { + + OPTION_COMMON_HELP: + return help(); + + OPTION_COMMON_VERSION_WITH_HIDDEN_V: + return version(); + + OPTION('d', "device", "PATH", "Device node for SG_IO commands"): + dev_specified = true; + strscpy(maj_min_dev, MAX_PATH_LEN, arg); + break; + + OPTION('f', "config", "PATH", "Location of config file"): + strscpy(config_file, MAX_PATH_LEN, arg); + break; + + OPTION('p', "page", "0x80|0x83|pre-spc3-83", "SCSI page"): + r = parse_page_code(arg, &default_page_code); + if (r < 0) + return r; + break; + + OPTION('s', "sg-version", "3|4", "Use SGv3 or SGv4"): + r = safe_atoi(arg, &sg_version); + if (r < 0) + return log_error_errno(r, "Invalid SG version '%s'", arg); + if (!IN_SET(sg_version, 3, 4)) + return log_error_errno(SYNTHETIC_ERRNO(ERANGE), + "Unknown SG version '%s'", arg); + break; + + OPTION('b', "denylisted", NULL, "Treat device as denylisted"): {} + OPTION('b', "blacklisted", NULL, /* help= */ NULL): /* backward compat */ all_good = false; break; - case 'd': - dev_specified = true; - strscpy(maj_min_dev, MAX_PATH_LEN, optarg); - break; - - case 'f': - strscpy(config_file, MAX_PATH_LEN, optarg); - break; - - case 'g': + OPTION('g', "allowlisted", NULL, "Treat device as allowlisted"): {} + OPTION('g', "whitelisted", NULL, /* help= */ NULL): /* backward compat */ all_good = true; break; - case 'h': - help(); - exit(EXIT_SUCCESS); - - case 'p': - if (streq(optarg, "0x80")) - default_page_code = PAGE_80; - else if (streq(optarg, "0x83")) - default_page_code = PAGE_83; - else if (streq(optarg, "pre-spc3-83")) - default_page_code = PAGE_83_PRE_SPC3; - else - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Unknown page code '%s'", - optarg); - break; - - case 's': - r = safe_atoi(optarg, &sg_version); - if (r < 0) - return log_error_errno(r, - "Invalid SG version '%s'", - optarg); - if (sg_version < 3 || sg_version > 4) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Unknown SG version '%s'", - optarg); - break; - - case 'u': + OPTION('u', "replace-whitespace", NULL, "Replace all whitespace by underscores"): reformat_serial = true; break; - case 'v': + OPTION('v', "verbose", NULL, "Verbose logging"): log_set_target(LOG_TARGET_CONSOLE); log_set_max_level(LOG_DEBUG); log_open(); break; - case 'V': - version(); - exit(EXIT_SUCCESS); - - case 'x': + OPTION('x', "export", NULL, "Print values as environment keys"): export = true; break; - - case '?': - return -1; - - default: - assert_not_reached(); } - if (optind < argc && !dev_specified) { + char **args = option_parser_get_args(&state); + if (!strv_isempty(args) && !dev_specified) { dev_specified = true; - strscpy(maj_min_dev, MAX_PATH_LEN, argv[optind]); + strscpy(maj_min_dev, MAX_PATH_LEN, args[0]); } - return 0; + return 1; } -static int per_dev_options(struct scsi_id_device *dev_scsi, int *good_bad, int *page_code) { - _cleanup_strv_free_ char **newargv = NULL; - int retval; - int newargc; - int option; +static int per_dev_options(struct scsi_id_device *dev_scsi, int *good_bad, enum page_code *page_code) { + int r; + + assert(dev_scsi); + assert(good_bad); + assert(page_code); *good_bad = all_good; *page_code = default_page_code; - retval = get_file_options(vendor_str, model_str, &newargc, &newargv); + _cleanup_strv_free_ char **newargv = NULL; + r = get_file_options(vendor_str, model_str, &newargv); + if (r <= 0) + return r; - optind = 1; /* reset this global extern */ - while (retval == 0) { - option = getopt_long(newargc, newargv, "bgp:", options, NULL); - if (option == -1) - break; + size_t newargc = strv_length(newargv); + if (newargc > INT_MAX) + return log_oom(); /* Close enough :) */ - switch (option) { - case 'b': + OptionParser state = { newargc, newargv }; + const Option *opt; + const char *arg; + + /* We reuse the option parser, but only a subset of the options is supported here. + * If any others are encountered, return an error. */ + + FOREACH_OPTION_FULL(&state, c, &opt, &arg, /* on_error= */ return c) + if (opt->short_code == 'b') *good_bad = 0; - break; - - case 'g': + else if (opt->short_code == 'g') *good_bad = 1; - break; + else if (opt->short_code == 'p') { + r = parse_page_code(arg, page_code); + if (r < 0) + return r; + } else + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Option %s not supported in the config file.", + strnull(option_get_synopsis("", opt, "/", /* show_metavar=*/ false))); - case 'p': - if (streq(optarg, "0x80")) { - *page_code = PAGE_80; - } else if (streq(optarg, "0x83")) { - *page_code = PAGE_83; - } else if (streq(optarg, "pre-spc3-83")) { - *page_code = PAGE_83_PRE_SPC3; - } else { - log_error("Unknown page code '%s'", optarg); - retval = -1; - } - break; - - default: - log_error("Unknown or bad option '%c' (0x%x)", option, (unsigned) option); - retval = -1; - } - } - - return retval; + return 0; } static int set_inq_values(struct scsi_id_device *dev_scsi, const char *path) { - int retval; + int r; dev_scsi->use_sg = sg_version; - retval = scsi_std_inquiry(dev_scsi, path); - if (retval) - return retval; + r = scsi_std_inquiry(dev_scsi, path); + if (r < 0) + return r; encode_devnode_name(dev_scsi->vendor, vendor_enc_str, sizeof(vendor_enc_str)); encode_devnode_name(dev_scsi->model, model_enc_str, sizeof(model_enc_str)); @@ -400,28 +367,25 @@ static bool scsi_string_is_valid(const char *s) { /* * scsi_id: try to get an id, if one is found, printf it to stdout. - * returns a value passed to exit() - 0 if printed an id, else 1. */ static int scsi_id(char *maj_min_dev) { struct scsi_id_device dev_scsi = {}; - int good_dev; - int page_code; - int retval = 0; + enum page_code page_code; + int good_dev, r; - if (set_inq_values(&dev_scsi, maj_min_dev) < 0) { - retval = 1; - goto out; - } + r = set_inq_values(&dev_scsi, maj_min_dev); + if (r < 0) + return r; /* get per device (vendor + model) options from the config file */ - per_dev_options(&dev_scsi, &good_dev, &page_code); - if (!good_dev) { - retval = 1; - goto out; - } + r = per_dev_options(&dev_scsi, &good_dev, &page_code); + if (r < 0) + return r; + if (!good_dev) + return -EIO; /* read serial number from mode pages (no values for optical drives) */ - scsi_get_serial(&dev_scsi, maj_min_dev, page_code, MAX_SERIAL_LEN); + (void) scsi_get_serial(&dev_scsi, maj_min_dev, page_code, MAX_SERIAL_LEN); if (export) { char serial_str[MAX_SERIAL_LEN]; @@ -453,13 +417,11 @@ static int scsi_id(char *maj_min_dev) { printf("ID_TARGET_PORT=%s\n", dev_scsi.tgpt_group); if (scsi_string_is_valid(dev_scsi.unit_serial_number)) printf("ID_SCSI_SERIAL=%s\n", dev_scsi.unit_serial_number); - goto out; + return 0; } - if (dev_scsi.serial[0] == '\0') { - retval = 1; - goto out; - } + if (dev_scsi.serial[0] == '\0') + return -ENODATA; if (reformat_serial) { char serial_str[MAX_SERIAL_LEN]; @@ -467,19 +429,17 @@ static int scsi_id(char *maj_min_dev) { udev_replace_whitespace(dev_scsi.serial, serial_str, sizeof(serial_str)-1); udev_replace_chars(serial_str, NULL); printf("%s\n", serial_str); - goto out; + return 0; } printf("%s\n", dev_scsi.serial); -out: - return retval; + return 0; } -int main(int argc, char **argv) { +static int run(int argc, char **argv) { _cleanup_strv_free_ char **newargv = NULL; - int retval = 0; char maj_min_dev[MAX_PATH_LEN]; - int newargc; + int r; (void) udev_parse_config(); log_setup(); @@ -487,35 +447,30 @@ int main(int argc, char **argv) { /* * Get config file options. */ - retval = get_file_options(NULL, NULL, &newargc, &newargv); - if (retval < 0) { - retval = 1; - goto exit; - } - if (retval == 0) { - assert(newargv); + r = get_file_options(NULL, NULL, &newargv); + if (r < 0) + return r; + if (r == 1) { + size_t newargc = strv_length(newargv); + if (newargc > INT_MAX) + return log_oom(); /* Close enough :) */ - if (set_options(newargc, newargv, maj_min_dev) < 0) { - retval = 2; - goto exit; - } + r = set_options(newargc, newargv, maj_min_dev); + if (r <= 0) + return r; } /* * Get command line options (overriding any config file settings). */ - if (set_options(argc, argv, maj_min_dev) < 0) - exit(EXIT_FAILURE); + r = set_options(argc, argv, maj_min_dev); + if (r <= 0) + return r; - if (!dev_specified) { - log_error("No device specified."); - retval = 1; - goto exit; - } + if (!dev_specified) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "No device specified."); - retval = scsi_id(maj_min_dev); - -exit: - log_close(); - return retval; + return scsi_id(maj_min_dev); } + +DEFINE_MAIN_FUNCTION(run); diff --git a/src/udev/scsi_id/scsi_serial.c b/src/udev/scsi_id/scsi_serial.c index 95268635cb..6d8974eaa3 100644 --- a/src/udev/scsi_id/scsi_serial.c +++ b/src/udev/scsi_id/scsi_serial.c @@ -15,6 +15,7 @@ #include #include "devnum-util.h" +#include "fd-util.h" #include "hexdecoct.h" #include "log.h" #include "random-util.h" @@ -762,30 +763,23 @@ static int do_scsi_page80_inquiry(struct scsi_id_device *dev_scsi, int fd, } int scsi_std_inquiry(struct scsi_id_device *dev_scsi, const char *devname) { - int fd; - unsigned char buf[SCSI_INQ_BUFF_LEN]; + unsigned char buf[SCSI_INQ_BUFF_LEN] = {}; struct stat statbuf; - int err = 0; + int r; - fd = open(devname, O_RDONLY | O_NONBLOCK | O_CLOEXEC | O_NOCTTY); - if (fd < 0) { - log_debug_errno(errno, "scsi_id: cannot open %s: %m", devname); - return 1; - } + _cleanup_close_ int fd = open(devname, O_RDONLY | O_NONBLOCK | O_CLOEXEC | O_NOCTTY); + if (fd < 0) + return log_debug_errno(errno, "scsi_id: cannot open %s: %m", devname); + + if (fstat(fd, &statbuf) < 0) + return log_debug_errno(errno, "scsi_id: cannot stat %s: %m", devname); - if (fstat(fd, &statbuf) < 0) { - log_debug_errno(errno, "scsi_id: cannot stat %s: %m", devname); - err = 2; - goto out; - } format_devnum(statbuf.st_rdev, dev_scsi->kernel); - memzero(buf, SCSI_INQ_BUFF_LEN); - err = scsi_inquiry(dev_scsi, fd, 0, 0, buf, SCSI_INQ_BUFF_LEN); - if (err < 0) - goto out; + r = scsi_inquiry(dev_scsi, fd, 0, 0, buf, SCSI_INQ_BUFF_LEN); + if (r < 0) + return r; - err = 0; memcpy(dev_scsi->vendor, buf + 8, 8); dev_scsi->vendor[8] = '\0'; memcpy(dev_scsi->model, buf + 16, 16); @@ -793,10 +787,7 @@ int scsi_std_inquiry(struct scsi_id_device *dev_scsi, const char *devname) { memcpy(dev_scsi->revision, buf + 32, 4); dev_scsi->revision[4] = '\0'; dev_scsi->type = buf[0] & 0x1f; - -out: - close(fd); - return err; + return 0; } int scsi_get_serial(struct scsi_id_device *dev_scsi, const char *devname, From 33a37278aa81c2e9c4cdc15d470f4e5ee0ab2dc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 25 Apr 2026 19:06:55 +0200 Subject: [PATCH 9/9] iocost: convert to the new option parser --help output only has changes expected in the new style. Co-developed-by: Claude Opus 4.7 (1M context) --- src/udev/iocost/iocost.c | 90 +++++++++++++++++++--------------------- 1 file changed, 43 insertions(+), 47 deletions(-) diff --git a/src/udev/iocost/iocost.c b/src/udev/iocost/iocost.c index 3d473d469d..0767ccba09 100644 --- a/src/udev/iocost/iocost.c +++ b/src/udev/iocost/iocost.c @@ -1,6 +1,5 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ -#include #include #include "sd-device.h" @@ -11,7 +10,10 @@ #include "conf-parser.h" #include "device-util.h" #include "devnum-util.h" +#include "format-table.h" +#include "help-util.h" #include "main-func.h" +#include "options.h" #include "string-util.h" #include "strv.h" #include "udev-util.h" @@ -50,53 +52,52 @@ static int parse_config(void) { } static int help(void) { - printf("%s [OPTIONS...]\n\n" - "Set up iocost model and qos solutions for block devices\n" - "\nCommands:\n" - " apply [SOLUTION] Apply solution for the device if\n" - " found, do nothing otherwise\n" - " query Query the known solution for\n" - " the device\n" - "\nOptions:\n" - " -h --help Show this help\n" - " --version Show package version\n", - program_invocation_short_name); + _cleanup_(table_unrefp) Table *options = NULL, *verbs = NULL; + int r; - return 0; + r = option_parser_get_help_table(&options); + if (r < 0) + return r; + + r = verbs_get_help_table(&verbs); + if (r < 0) + return r; + + (void) table_sync_column_widths(0, options, verbs); + + help_cmdline("[OPTIONS...] COMMAND"); + help_abstract("Set up iocost model and qos solutions for block devices."); + + help_section("Commands:"); + r = table_print_or_warn(verbs); + if (r < 0) + return r; + + help_section("Options:"); + return table_print_or_warn(options); } -static int parse_argv(int argc, char *argv[]) { - enum { - ARG_VERSION = 0x100, - }; +VERB_COMMON_HELP_HIDDEN(help); - static const struct option options[] = { - { "help", no_argument, NULL, 'h' }, - { "version", no_argument, NULL, ARG_VERSION }, - {} - }; - - int c; - - assert(argc >= 1); +static int parse_argv(int argc, char *argv[], char ***remaining_args) { + assert(argc >= 0); assert(argv); + assert(remaining_args); - while ((c = getopt_long(argc, argv, "h", options, NULL)) >= 0) + OptionParser state = { argc, argv }; + const char *arg; + + FOREACH_OPTION(&state, c, &arg, /* on_error= */ return c) switch (c) { - case 'h': + OPTION_COMMON_HELP: return help(); - case ARG_VERSION: + OPTION_COMMON_VERSION: return version(); - - case '?': - return -EINVAL; - - default: - assert_not_reached(); } + *remaining_args = option_parser_get_args(&state); return 1; } @@ -280,32 +281,27 @@ static int query_solutions_for_path(const char *path) { return 0; } +VERB(verb_query, "query", "PATH", 2, 2, 0, + "Query the known solution for the device"); static int verb_query(int argc, char *argv[], uintptr_t _data, void *userdata) { return query_solutions_for_path(ASSERT_PTR(argv[1])); } +VERB(verb_apply, "apply", "PATH [SOLUTION]", 2, 3, 0, + "Apply solution for the device if found, do nothing otherwise"); static int verb_apply(int argc, char *argv[], uintptr_t _data, void *userdata) { return apply_solution_for_path( ASSERT_PTR(argv[1]), argc > 2 ? ASSERT_PTR(argv[2]) : NULL); } -static int iocost_main(int argc, char *argv[]) { - static const Verb verbs[] = { - { "query", 2, 2, 0, verb_query }, - { "apply", 2, 3, 0, verb_apply }, - {}, - }; - - return dispatch_verb(argc, argv, verbs, NULL); -} - static int run(int argc, char *argv[]) { + char **args = NULL; int r; log_setup(); - r = parse_argv(argc, argv); + r = parse_argv(argc, argv, &args); if (r <= 0) return r; @@ -313,7 +309,7 @@ static int run(int argc, char *argv[]) { if (r < 0) return r; - return iocost_main(argc, argv); + return dispatch_verb_with_args(args, NULL); } DEFINE_MAIN_FUNCTION(run);