]> git.ipfire.org Git - thirdparty/systemd.git/commitdiff
scsi_id: convert to the new option parser
authorZbigniew Jędrzejewski-Szmek <zbyszek@amutable.com>
Sat, 25 Apr 2026 11:01:13 +0000 (13:01 +0200)
committerZbigniew Jędrzejewski-Szmek <zbyszek@amutable.com>
Mon, 27 Apr 2026 07:14:50 +0000 (09:14 +0200)
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
src/udev/scsi_id/scsi_serial.c

index ba7a7fe5f522def1bbefcffd5f910ba7d629d0cd..8f580a8cec6f581d0d7d054535b42329b7c71473 100644 (file)
@@ -5,7 +5,6 @@
  */
 
 #include <ctype.h>
-#include <getopt.h>
 #include <stdio.h>
 #include <stdlib.h>
 
 #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"
 #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':
-                        all_good = false;
-                        break;
+        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();
 
-                case 'd':
+                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, optarg);
+                        strscpy(maj_min_dev, MAX_PATH_LEN, arg);
                         break;
 
-                case 'f':
-                        strscpy(config_file, MAX_PATH_LEN, optarg);
+                OPTION('f', "config", "PATH", "Location of config file"):
+                        strscpy(config_file, MAX_PATH_LEN, arg);
                         break;
 
-                case 'g':
-                        all_good = true;
+                OPTION('p', "page", "0x80|0x83|pre-spc3-83", "SCSI page"):
+                        r = parse_page_code(arg, &default_page_code);
+                        if (r < 0)
+                                return r;
                         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);
+                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;
 
-                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);
+                OPTION('b', "denylisted", NULL, "Treat device as denylisted"): {}
+                OPTION('b', "blacklisted", NULL, /* help= */ NULL): /* backward compat */
+                        all_good = false;
+                        break;
+
+                OPTION('g', "allowlisted", NULL, "Treat device as allowlisted"): {}
+                OPTION('g', "whitelisted", NULL, /* help= */ NULL): /* backward compat */
+                        all_good = true;
                         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);
-
-        optind = 1; /* reset this global extern */
-        while (retval == 0) {
-                option = getopt_long(newargc, newargv, "bgp:", options, NULL);
-                if (option == -1)
-                        break;
+        _cleanup_strv_free_ char **newargv = NULL;
+        r = get_file_options(vendor_str, model_str, &newargv);
+        if (r <= 0)
+                return r;
 
-                switch (option) {
-                case 'b':
-                        *good_bad = 0;
-                        break;
+        size_t newargc = strv_length(newargv);
+        if (newargc > INT_MAX)
+                return log_oom();  /* Close enough :) */
 
-                case 'g':
-                        *good_bad = 1;
-                        break;
+        OptionParser state = { newargc, newargv };
+        const Option *opt;
+        const char *arg;
 
-                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;
+        /* We reuse the option parser, but only a subset of the options is supported here.
+         * If any others are encountered, return an error. */
 
-                default:
-                        log_error("Unknown or bad option '%c' (0x%x)", option, (unsigned) option);
-                        retval = -1;
-                }
-        }
+        FOREACH_OPTION_FULL(&state, c, &opt, &arg, /* on_error= */ return c)
+                if (opt->short_code == 'b')
+                        *good_bad = 0;
+                else if (opt->short_code == 'g')
+                        *good_bad = 1;
+                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)));
 
-        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);
-
-                if (set_options(newargc, newargv, maj_min_dev) < 0) {
-                        retval = 2;
-                        goto exit;
-                }
+        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 :) */
+
+                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);
-
-        if (!dev_specified) {
-                log_error("No device specified.");
-                retval = 1;
-                goto exit;
-        }
+        r = set_options(argc, argv, maj_min_dev);
+        if (r <= 0)
+                return r;
 
-        retval = scsi_id(maj_min_dev);
+        if (!dev_specified)
+                return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "No device specified.");
 
-exit:
-        log_close();
-        return retval;
+        return scsi_id(maj_min_dev);
 }
+
+DEFINE_MAIN_FUNCTION(run);
index 95268635cb637235968da23ab6c7bbc3f9598b04..6d8974eaa39677ba55f548051655901b5892655c 100644 (file)
@@ -15,6 +15,7 @@
 #include <unistd.h>
 
 #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,