Re: [RFC PATCH 05/11] lib: getopt: Permute by default with inline reorder

[Sean Anderson <seanga2@gmail.com>]

On 5/15/26 16:32, Simon Glass wrote:
> Currently getopt() does not reorder its argv: it stops at the first
> non-option, which forces options to appear before positional arguments.
> This is not the usual way that arguments work - people expect options to
> be recognised wherever they appear on the command line, although some
> U-Boot commands are hard-wires for particular positions.
> 
> Restructure the parser to permute by default. Add argc and a writable
> args[CONFIG_SYS_MAXARGS + 1] buffer to struct getopt_state, plus a
> nonopts counter. The init function getopt_init_state() takes argc and
> argv, copies argv into args, and resets the parse position. Then
> getopt() and getopt_silent() take only the state and an optstring; argc
> and argv are read from the state.
> 
> When getopt() encounters a non-option, it bubbles that argv element to
> the end of args and increments nonopts, then keeps scanning. The outer
> condition stops when index + nonopts reaches argc, so the parked
> non-options are never re-scanned. After parsing finishes, gs.index
> points at the first parked non-option, with gs.nonopts of them sitting
> at gs.args[gs.index .. argc - 1]
> 
> Callers that need the previous POSIX-style 'stop at first non-option'
> behaviour can prefix optstring with '+'. This matches GNU getopt's
> convention and keeps to two public entry points (getopt() and
> getopt_silent()) instead of doubling them up for in-order variants.
> 
> Update the two existing in-tree callers, cmd/bdinfo.c and cmd/log.c, to
> the new signatures. bdinfo has no positional arguments so it works the
> same either way; log filter-list/add/remove preserve their original
> semantics with the '+' prefix. Update test helper in test/lib/getopt.c
> too.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>   cmd/bdinfo.c      |   4 +-
>   cmd/log.c         |  12 ++--
>   include/getopt.h  | 146 +++++++++++++++++++++++-----------------------
>   lib/getopt.c      |  67 ++++++++++++++++-----
>   test/lib/getopt.c |   6 +-
>   5 files changed, 135 insertions(+), 100 deletions(-)
> 
> diff --git a/cmd/bdinfo.c b/cmd/bdinfo.c
> index ddf77303735..c46094c3ddf 100644
> --- a/cmd/bdinfo.c
> +++ b/cmd/bdinfo.c
> @@ -188,8 +188,8 @@ int do_bdinfo(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   	if (!CONFIG_IS_ENABLED(GETOPT) || argc == 1)
>   		return bdinfo_print_all(bd);
>   
> -	getopt_init_state(&gs);
> -	while ((opt = getopt(&gs, argc, argv, "aem")) > 0) {
> +	getopt_init_state(&gs, argc, argv);
> +	while ((opt = getopt(&gs, "aem")) > 0) {
>   		switch (opt) {
>   		case 'a':
>   			return bdinfo_print_all(bd);
> diff --git a/cmd/log.c b/cmd/log.c
> index 64add6d8b5a..344f379d8cc 100644
> --- a/cmd/log.c
> +++ b/cmd/log.c
> @@ -95,8 +95,8 @@ static int do_log_filter_list(struct cmd_tbl *cmdtp, int flag, int argc,
>   	struct log_filter *filt;
>   	struct log_device *ldev;
>   
> -	getopt_init_state(&gs);
> -	while ((opt = getopt(&gs, argc, argv, "d:")) > 0) {
> +	getopt_init_state(&gs, argc, argv);
> +	while ((opt = getopt(&gs, "+d:")) > 0) {
>   		switch (opt) {
>   		case 'd':
>   			drv_name = gs.arg;
> @@ -157,8 +157,8 @@ static int do_log_filter_add(struct cmd_tbl *cmdtp, int flag, int argc,
>   	enum log_level_t level = LOGL_MAX;
>   	struct getopt_state gs;
>   
> -	getopt_init_state(&gs);
> -	while ((opt = getopt(&gs, argc, argv, "Ac:d:Df:F:l:L:p")) > 0) {
> +	getopt_init_state(&gs, argc, argv);
> +	while ((opt = getopt(&gs, "+Ac:d:Df:F:l:L:p")) > 0) {
>   		switch (opt) {
>   		case 'A':
>   #define do_type() do { \
> @@ -250,8 +250,8 @@ static int do_log_filter_remove(struct cmd_tbl *cmdtp, int flag, int argc,
>   	const char *drv_name = "console";
>   	struct getopt_state gs;
>   
> -	getopt_init_state(&gs);
> -	while ((opt = getopt(&gs, argc, argv, "ad:")) > 0) {
> +	getopt_init_state(&gs, argc, argv);
> +	while ((opt = getopt(&gs, "+ad:")) > 0) {
>   		switch (opt) {
>   		case 'a':
>   			all = true;
> diff --git a/include/getopt.h b/include/getopt.h
> index 0cf7ee84d6f..5a9e7802e65 100644
> --- a/include/getopt.h
> +++ b/include/getopt.h
> @@ -10,120 +10,120 @@
>   #define __GETOPT_H
>   
>   #include <stdbool.h>
> +#include <linux/kconfig.h>
>   
>   /**
>    * struct getopt_state - Saved state across getopt() calls
> + *
> + * Initialise with getopt_init_state(); the embedded @args buffer holds a
> + * working copy of the argv passed to that initialiser.
>    */
>   struct getopt_state {
> +	/** @argc: Argument count, as passed to getopt_init_state() */
> +	int argc;
> +	/**
> +	 * @args: Working copy of argv. getopt() reorders this in place: as
> +	 * options are consumed, non-options are bubbled to the end.

Please use terminology from getopt(3) wherever possible. So "permute" instead
of "bubble" or "park".

non-options are permuted to the end.

> +	 */
> +	char *args[CONFIG_SYS_MAXARGS + 1];

This should come first to eliminate padding. And should probably be named argv for consistency.

>   	/**
> -	 * @index: Index of the next unparsed argument of @argv. If getopt() has
> -	 * parsed all of @argv, then @index will equal @argc.
> +	 * @index: Index of the next unparsed argument of @args. When
> +	 * parsing has finished, @index sits at the first parked
> +	 * non-option (or @argc if none).

Index of the next unparsed argument of @argv. If getopt() has parsed all of @argv,
then @index will be the first non-option argument of @args (or @argc if none).

>   	 */
>   	int index;
> -	/** @arg_index: Index within the current argument */
> +	/**
> +	 * @arg_index: Offset of the next unparsed character within
> +	 * ``args[index]``. Lets the parser step through grouped short
> +	 * options like ``-abc`` (1, 2, 3...). Reset to 1 each time
> +	 * @index advances to the next argv slot.

to the next argument.

> +	 */
>   	int arg_index;
> +	/**
> +	 * @nonopts: Number of non-option arguments parked at the tail> +	 * of @args. The parser bubbles them down so it can keep scanning
> +	 * for options past them.

Number of non-option arguments in @args.

> +	 */
> +	int nonopts;

Could we just modify argc instead? E.g. once we move a non-option to the end, then we
decrement argc. I think this also fixes the problem discussed below.

>   	union {
>   		/**
> -		 * @opt: Option being parsed when an error occurs. @opt is only
> -		 * valid when getopt() returns ``?`` or ``:``.
> +		 * @opt: Option being parsed when an error occurs. @opt is
> +		 * only valid when getopt() returns ``?`` or ``:``.
>   		 */
>   		int opt;
>   		/**
> -		 * @arg: The argument to an option, NULL if there is none. @arg
> -		 * is only valid when getopt() returns an option character.
> +		 * @arg: The argument to an option, NULL if there is none.
> +		 * @arg is only valid when getopt() returns an option
> +		 * character.
>   		 */
>   		char *arg;
>   	};
>   };
>   
>   /**
> - * getopt_init_state() - Initialize a &struct getopt_state
> - * @gs: The state to initialize
> - *
> - * This must be called before using @gs with getopt().
> + * getopt_init_state() - Initialise a &struct getopt_state from an argv
> + * @gs: The state to initialise
> + * @argc: Argument count
> + * @argv: Source argv. Copied into @gs->args; the original is not
> + *        modified. @argc must not exceed ``CONFIG_SYS_MAXARGS``;
> + *        excess arguments are silently dropped.
>    */
> -void getopt_init_state(struct getopt_state *gs);
> +void getopt_init_state(struct getopt_state *gs, int argc,
> +		       char *const argv[]);
>   
> -int __getopt(struct getopt_state *gs, int argc, char *const argv[],
> -	     const char *optstring, bool silent);
> +int __getopt(struct getopt_state *gs, const char *optstring, bool silent);
>   
>   /**
>    * getopt() - Parse short command-line options
> - * @gs: Internal state and out-of-band return arguments. This must be
> - *      initialized with getopt_init_context() beforehand.
> - * @argc: Number of arguments, not including the %NULL terminator
> - * @argv: Argument list, terminated by %NULL
> - * @optstring: Option specification, as described below
> - *
> - * getopt() parses short options. Short options are single characters. They may
> - * be followed by a required argument or an optional argument. Arguments to
> - * options may occur in the same argument as an option (like ``-larg``), or
> - * in the following argument (like ``-l arg``). An argument containing
> - * options begins with a ``-``. If an option expects no arguments, then it may
> - * be immediately followed by another option (like ``ls -alR``).
> - *
> - * @optstring is a list of accepted options. If an option is followed by ``:``
> - * in @optstring, then it expects a mandatory argument. If an option is followed
> - * by ``::`` in @optstring, it expects an optional argument. @gs.arg points
> - * to the argument, if one is parsed.
> - *
> - * getopt() stops parsing options when it encounters the first non-option
> - * argument, when it encounters the argument ``--``, or when it runs out of
> - * arguments. For example, in ``ls -l foo -R``, option parsing will stop when
> - * getopt() encounters ``foo``, if ``l`` does not expect an argument. However,
> - * the whole list of arguments would be parsed if ``l`` expects an argument.
> + * @gs: State, initialised with getopt_init_state()
> + * @optstring: Option specification (see getopt(3))

Please retain the original description. We are not 100% compatible
with getopt, and I think explicit examples are a good way to document what we
support as well as the new API, which doesn't quite match libc getopt.

>    *
> - * An example invocation of getopt() might look like::
> + * Parses the next short option from @gs. By default, getopt() permutes

Parse

> + * the working argv: when it hits a non-option, it bubbles that argv

bubbles -> moves

> + * element to the end and keeps scanning, so options can appear
> + * anywhere on the command line. After parsing finishes (returns -1),
> + * @gs.index sits at the first parked non-option, and there are
> + * @gs.nonopts of them at @gs.args[gs.index..argc-1].
>    *
> - *     char *argv[] = { "program", "-cbx", "-a", "foo", "bar", 0 };
> - *     int opt, argc = ARRAY_SIZE(argv) - 1;
> - *     struct getopt_state gs;
> + * If @optstring begins with ``+``, getopt() instead stops at the
> + * first non-option (POSIX ``getopt`` behaviour). Use this when the
> + * command's grammar requires options before positionals, or when the
> + * option string includes optional arguments (``::``) whose adjacency
> + * to a following positional would be ambiguous under permutation.

when the command requires options before non-options, or when options
with optional arguments would be ambiguous under permutation.

>    *
> - *     getopt_init_state(&gs);
> - *     while ((opt = getopt(&gs, argc, argv, "a::b:c")) != -1)
> - *         printf("opt = %c, index = %d, arg = \"%s\"\n", opt, gs.index, gs.arg);
> - *     printf("%d argument(s) left\n", argc - gs.index);
> + * In @optstring proper, ``x`` is a flag, ``x:`` requires an argument,
> + * and ``x::`` takes an optional argument. The argument is delivered
> + * in @gs.arg.
>   *
> - * and would produce an output of::
> - *
> - *     opt = c, index = 1, arg = "<NULL>"
> - *     opt = b, index = 2, arg = "x"
> - *     opt = a, index = 4, arg = "foo"
> - *     1 argument(s) left
> - *
> - * For further information, refer to the getopt(3) man page.
> + * A literal ``--`` argument terminates option scanning; the parser
> + * advances past it and returns -1.
>    *
>    * Return:
> - * * An option character if an option is found. @gs.arg is set to the
> - *   argument if there is one, otherwise it is set to ``NULL``.
> - * * ``-1`` if there are no more options, if a non-option argument is
> - *   encountered, or if an ``--`` argument is encountered.
> - * * ``'?'`` if we encounter an option not in @optstring. @gs.opt is set to
> - *   the unknown option.
> - * * ``':'`` if an argument is required, but no argument follows the
> - *   option. @gs.opt is set to the option missing its argument.
> - *
> - * @gs.index is always set to the index of the next unparsed argument in @argv.
> + * * An option character if an option is found. @gs.arg is set to
> + *   the argument if there is one, otherwise NULL.
> + * * ``-1`` if there are no more options.
> + * * ``'?'`` for an option not in @optstring; @gs.opt is the unknown
> + *   option character.
> + * * ``':'`` for an option missing its required argument; @gs.opt is
> + *   the option character.
>    */
> -static inline int getopt(struct getopt_state *gs, int argc,
> -			 char *const argv[], const char *optstring)
> +static inline int getopt(struct getopt_state *gs, const char *optstring)
>   {
> -	return __getopt(gs, argc, argv, optstring, false);
> +	return __getopt(gs, optstring, false);
>   }
>   
>   /**
> - * getopt_silent() - Parse short command-line options silently
> + * getopt_silent() - Parse short command-line options without errors
>    * @gs: State
> - * @argc: Argument count
> - * @argv: Argument list
>    * @optstring: Option specification
>    *
> - * Same as getopt(), except no error messages are printed.
> + * Same as getopt() but does not print error messages for unknown
> + * options or missing arguments.
>    */
> -static inline int getopt_silent(struct getopt_state *gs, int argc,
> -				char *const argv[], const char *optstring)
> +static inline int getopt_silent(struct getopt_state *gs,
> +				const char *optstring)
>   {
> -	return __getopt(gs, argc, argv, optstring, true);
> +	return __getopt(gs, optstring, true);
>   }
>   
>   #endif /* __GETOPT_H */
> diff --git a/lib/getopt.c b/lib/getopt.c
> index e9175e2fff4..70adb2d0faf 100644
> --- a/lib/getopt.c
> +++ b/lib/getopt.c
> @@ -10,37 +10,71 @@
>   
>   #include <getopt.h>
>   #include <log.h>
> +#include <linux/kernel.h>
>   #include <linux/string.h>
>   
> -void getopt_init_state(struct getopt_state *gs)
> +void getopt_init_state(struct getopt_state *gs, int argc, char *const argv[])
>   {
> +	int max = ARRAY_SIZE(gs->args) - 1;
> +
> +	if (argc > max)
> +		argc = max;
> +
> +	gs->argc = argc;
> +	memcpy(gs->args, argv, (argc + 1) * sizeof(*gs->args));
> +	gs->args[argc] = NULL;
>   	gs->index = 1;
>   	gs->arg_index = 1;
> +	gs->nonopts = 0;
>   }
>   
> -int __getopt(struct getopt_state *gs, int argc, char *const argv[],
> -	     const char *optstring, bool silent)
> +int __getopt(struct getopt_state *gs, const char *optstring, bool silent)
>   {
> -	char curopt;   /* current option character */
> -	const char *curoptp; /* pointer to the current option in optstring */
> +	char curopt;	/* current option character */
> +	const char *curoptp;	/* pointer to the current option in optstring */
> +	bool stop_nonopt = false;
> +	char **argv = gs->args;
> +	int argc = gs->argc;
> +
> +	if (*optstring == '+') {
> +		stop_nonopt = true;
> +		optstring++;
> +	}
>   
>   	while (1) {
> -		log_debug("arg_index: %d index: %d\n", gs->arg_index,
> -			  gs->index);
> +		log_debug("arg_index: %d index: %d nonopts: %d\n",
> +			  gs->arg_index, gs->index, gs->nonopts);
>   
>   		/* `--` indicates the end of options */
> -		if (gs->arg_index == 1 && argv[gs->index] &&
> +		if (gs->arg_index == 1 && gs->index < argc &&
>   		    !strcmp(argv[gs->index], "--")) {
>   			gs->index++;
>   			return -1;
>   		}
>   
> -		/* Out of arguments */
> -		if (gs->index >= argc)
> -			return -1;
> +		/*
> +		 * Bubble non-options to the end so we can keep scanning for
> +		 * options past them. In '+' mode (POSIX), stop at the first
> +		 * non-option instead.
> +		 */
> +		while (gs->arg_index == 1 &&
> +		       gs->index + gs->nonopts < argc &&
> +		       *argv[gs->index] != '-') {
> +			char *tmp;
> +			int i;
> +
> +			if (stop_nonopt)
> +				return -1;
> +
> +			tmp = argv[gs->index];
> +			gs->nonopts++;
> +			for (i = gs->index; i + 1 < argc; i++)
> +				argv[i] = argv[i + 1];
> +			argv[argc - 1] = tmp;
> +		}
>   
> -		/* Can't parse non-options */
> -		if (*argv[gs->index] != '-')
> +		/* Out of options to scan */
> +		if (gs->index + gs->nonopts >= argc)
>   			return -1;
>   
>   		/* We have found an option */
> @@ -48,7 +82,8 @@ int __getopt(struct getopt_state *gs, int argc, char *const argv[],
>   		if (curopt)
>   			break;
>   		/*
> -		 * no more options in current argv[] element; try the next one
> +		 * No more options in current argv[] element; advance to the
> +		 * next one
>   		 */
>   		gs->index++;
>   		gs->arg_index = 1;
> @@ -80,7 +115,7 @@ int __getopt(struct getopt_state *gs, int argc, char *const argv[],
>   			gs->arg_index = 1;
>   			return curopt;
>   		}

Is the above condition (not included in the diff) correct for something like

getopt({ "prog", "foo", "-a" }, 3, "a:")

? That is, getopt should return ':' and not 'a' in this situation, but I'm not sure we're
tracking the non-options correctly.

> -		if (gs->index + 1 == argc) {
> +		if (gs->index + gs->nonopts + 1 == argc) {
>   			/* We are at the last argv[] element */
>   			gs->arg = NULL;
>   			gs->index++;
> @@ -113,7 +148,7 @@ int __getopt(struct getopt_state *gs, int argc, char *const argv[],
>   	gs->index++;
>   	gs->arg_index = 1;
>   
> -	if (gs->index >= argc || argv[gs->index][0] == '-') {
> +	if (gs->index + gs->nonopts >= argc || argv[gs->index][0] == '-') {
>   		if (!silent)
>   			printf("option requires an argument -- %c\n", curopt);
>   		gs->opt = curopt;
> diff --git a/test/lib/getopt.c b/test/lib/getopt.c
> index 388a076200b..6b384eb016a 100644
> --- a/test/lib/getopt.c
> +++ b/test/lib/getopt.c
> @@ -18,9 +18,9 @@ static int do_test_getopt(struct unit_test_state *uts, int line,
>   {
>   	int opt;
>   
> -	getopt_init_state(gs);
> +	getopt_init_state(gs, args, argv);
>   	for (int i = 0; i < expected_count; i++) {
> -		opt = getopt_silent(gs, args, argv, optstring);
> +		opt = getopt_silent(gs, optstring);
>   		if (expected[i] != opt) {
>   			/*
>   			 * Fudge the line number so we can tell which test
> @@ -34,7 +34,7 @@ static int do_test_getopt(struct unit_test_state *uts, int line,
>   		}
>   	}
>   
> -	opt = getopt_silent(gs, args, argv, optstring);
> +	opt = getopt_silent(gs, optstring);
>   	if (opt != -1) {
>   		ut_failf(uts, __FILE__, line, __func__,
>   			 "getopt() != -1",

Please add a few tests for reordered arguments, especially the scenario described above.