git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 2/5] parse-options: introduce precision handling for `OPTION_INTEGER`

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Apr 15, 2025, 15:51 UTC
Message-ID
<8e566ea1-5ff4-4854-a1dc-38626510c080@gmail.com>
In-Reply-To
<20250415-b4-pks-parse-options-integers-v2-2-ce07441a1f01@pks.im>
Hi Patrick
On 15/04/2025 13:14, Patrick Steinhardt wrote:
Show 8 quoted lines
> 
> Improve the situation by introducing a new `precision` field into the
> structure. This field gets assigned automatically by `OPT_INTEGER_F()`
> and tracks the size of the passed value. Like this it becomes possible
> for the caller to pass arbitrarily-sized integers and the underlying
> logic knows to handle it correctly by doing range checks. Furthermore,
> convert the code to use `strtoimax()` intstead of `strtol()` so that we
> can also parse values larger than `LONG_MAX`.
Nice, this is a really useful improvement. I've left one comment below
Show 8 quoted lines
> Note that we do not yet assert signedness of the passed variable, which
> is another source of bugs. This will be handled in a subsequent commit.
> 
> +		} else {
> +			value = strtoimax(arg, (char **)&s, 10);
> +			if (*s)
> +				return error(_("%s expects a numerical value"),
> +					     optname(opt, flags));
To catch overflow errors for arguments of intimax_t we need to do
	errno = 0
	value = strtoimax(arg, (Char **)&s, 10);
	if (errno || *s)
		return error(...)

to catch the error when we parse the string as the checks below only work for narrower types.

Best Wishes
Phillip
Show 190 quoted lines
> +
> +		}
>   
> +		if (value < lower_bound || value > upper_bound)
> +			return error(_("value %"PRIdMAX" for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
> +				     value, optname(opt, flags), lower_bound, upper_bound);
> +
> +		switch (opt->precision) {
> +		case 1:
> +			*(int8_t *)opt->value = value;
> +			return 0;
> +		case 2:
> +			*(int16_t *)opt->value = value;
> +			return 0;
> +		case 4:
> +			*(int32_t *)opt->value = value;
> +			return 0;
> +		case 8:
> +			*(int64_t *)opt->value = value;
> +			return 0;
> +		default:
> +			BUG("invalid precision for option %s",
> +			    optname(opt, flags));
> +		}
> +	}
>   	case OPTION_MAGNITUDE:
>   		if (unset) {
>   			*(unsigned long *)opt->value = 0;
> diff --git a/parse-options.h b/parse-options.h
> index 997ffbee805..8d5f9c95f9c 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -92,6 +92,10 @@ typedef int parse_opt_subcommand_fn(int argc, const char **argv,
>    * `value`::
>    *   stores pointers to the values to be filled.
>    *
> + * `precision`::
> + *   precision of the integer pointed to by `value`. Should typically be its
> + *   `sizeof()`.
> + *
>    * `argh`::
>    *   token to explain the kind of argument this option wants. Does not
>    *   begin in capital letter, and does not end with a full stop.
> @@ -151,6 +155,7 @@ struct option {
>   	int short_name;
>   	const char *long_name;
>   	void *value;
> +	size_t precision;
>   	const char *argh;
>   	const char *help;
>   
> @@ -214,6 +219,7 @@ struct option {
>   	.short_name = (s), \
>   	.long_name = (l), \
>   	.value = (v), \
> +	.precision = sizeof(*v), \
>   	.argh = N_("n"), \
>   	.help = (h), \
>   	.flags = (f), \
> diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
> index 997f55fd45b..b1275dfade4 100644
> --- a/t/helper/test-parse-options.c
> +++ b/t/helper/test-parse-options.c
> @@ -120,6 +120,7 @@ int cmd__parse_options(int argc, const char **argv)
>   	};
>   	struct string_list expect = STRING_LIST_INIT_NODUP;
>   	struct string_list list = STRING_LIST_INIT_NODUP;
> +	int16_t i16 = 0;
>   
>   	struct option options[] = {
>   		OPT_BOOL(0, "yes", &boolean, "get a boolean"),
> @@ -139,6 +140,7 @@ int cmd__parse_options(int argc, const char **argv)
>   		OPT_NEGBIT(0, "neg-or4", &boolean, "same as --no-or4", 4),
>   		OPT_GROUP(""),
>   		OPT_INTEGER('i', "integer", &integer, "get a integer"),
> +		OPT_INTEGER(0, "i16", &i16, "get a 16 bit integer"),
>   		OPT_INTEGER('j', NULL, &integer, "get a integer, too"),
>   		OPT_MAGNITUDE('m', "magnitude", &magnitude, "get a magnitude"),
>   		OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23),
> @@ -210,6 +212,7 @@ int cmd__parse_options(int argc, const char **argv)
>   	}
>   	show(&expect, &ret, "boolean: %d", boolean);
>   	show(&expect, &ret, "integer: %d", integer);
> +	show(&expect, &ret, "i16: %"PRIdMAX, (intmax_t) i16);
>   	show(&expect, &ret, "magnitude: %lu", magnitude);
>   	show(&expect, &ret, "timestamp: %"PRItime, timestamp);
>   	show(&expect, &ret, "string: %s", string ? string : "(not set)");
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index 2fe3522305f..e3ca7a27738 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -22,6 +22,7 @@ usage: test-tool parse-options <options>
>   
>       -i, --[no-]integer <n>
>                             get a integer
> +    --[no-]i16 <n>        get a 16 bit integer
>       -j <n>                get a integer, too
>       -m, --magnitude <n>   get a magnitude
>       --[no-]set23          set integer to 23
> @@ -136,6 +137,7 @@ test_expect_success 'OPT_MAGNITUDE() 3giga' '
>   cat >expect <<\EOF
>   boolean: 2
>   integer: 1729
> +i16: 0
>   magnitude: 16384
>   timestamp: 0
>   string: 123
> @@ -156,6 +158,7 @@ test_expect_success 'short options' '
>   cat >expect <<\EOF
>   boolean: 2
>   integer: 1729
> +i16: 9000
>   magnitude: 16384
>   timestamp: 0
>   string: 321
> @@ -167,7 +170,7 @@ file: prefix/fi.le
>   EOF
>   
>   test_expect_success 'long options' '
> -	test-tool parse-options --boolean --integer 1729 --magnitude 16k \
> +	test-tool parse-options --boolean --integer 1729 --i16 9000 --magnitude 16k \
>   		--boolean --string2=321 --verbose --verbose --no-dry-run \
>   		--abbrev=10 --file fi.le --obsolete \
>   		>output 2>output.err &&
> @@ -179,6 +182,7 @@ test_expect_success 'abbreviate to something longer than SHA1 length' '
>   	cat >expect <<-EOF &&
>   	boolean: 0
>   	integer: 0
> +	i16: 0
>   	magnitude: 0
>   	timestamp: 0
>   	string: (not set)
> @@ -253,6 +257,7 @@ test_expect_success 'superfluous value provided: cmdmode' '
>   cat >expect <<\EOF
>   boolean: 1
>   integer: 13
> +i16: 0
>   magnitude: 0
>   timestamp: 0
>   string: 123
> @@ -276,6 +281,7 @@ test_expect_success 'intermingled arguments' '
>   cat >expect <<\EOF
>   boolean: 0
>   integer: 2
> +i16: 0
>   magnitude: 0
>   timestamp: 0
>   string: (not set)
> @@ -343,6 +349,7 @@ cat >expect <<\EOF
>   Callback: "four", 0
>   boolean: 5
>   integer: 4
> +i16: 0
>   magnitude: 0
>   timestamp: 0
>   string: (not set)
> @@ -368,6 +375,7 @@ test_expect_success 'OPT_CALLBACK() and callback errors work' '
>   cat >expect <<\EOF
>   boolean: 1
>   integer: 23
> +i16: 0
>   magnitude: 0
>   timestamp: 0
>   string: (not set)
> @@ -447,6 +455,7 @@ test_expect_success 'OPT_NUMBER_CALLBACK() works' '
>   cat >expect <<\EOF
>   boolean: 0
>   integer: 0
> +i16: 0
>   magnitude: 0
>   timestamp: 0
>   string: (not set)
> @@ -783,4 +792,16 @@ test_expect_success 'magnitude with units but no numbers' '
>   	test_must_be_empty out
>   '
>   
> +test_expect_success 'i16 limits range' '
> +	test-tool parse-options --i16 32767 >out &&
> +	test_grep "i16: 32767" out &&
> +	test_must_fail test-tool parse-options --i16 32768 2>err &&
> +	test_grep "value 32768 for option .i16. not in range \[-32768,32767\]" err &&
> +
> +	test-tool parse-options --i16 -32768 >out &&
> +	test_grep "i16: -32768" out &&
> +	test_must_fail test-tool parse-options --i16 -32769 2>err &&
> +	test_grep "value -32769 for option .i16. not in range \[-32768,32767\]" err
> +'
> +
>   test_done
>
Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 12 of 37 in “parse-options: harden handling of integer values”
  1. 0/5 parse-options: harden handling of integer valuesPatrick Steinhardt, Apr 1, 2025
  2. 2/5 parse-options: introduce precision handling for `OPTION_INTEGER`Patrick Steinhardt, Apr 1, 2025
  3. René ScharfeApr 1, 2025
  4. Patrick SteinhardtApr 15, 2025
  5. 3/5 parse-options: introduce precision handling for `OPTION_MAGNITUDE`Patrick Steinhardt, Apr 1, 2025
  6. 4/5 parse-options: introduce `OPTION_UNSIGNED`Patrick Steinhardt, Apr 1, 2025
  7. 1/5 global: use designated initializers for optionsPatrick Steinhardt, Apr 1, 2025
  8. 5/5 parse-options: detect mismatches in integer signednessPatrick Steinhardt, Apr 1, 2025
  9. 0/5 parse-options: harden handling of integer valuesPatrick Steinhardt, Apr 15, 2025
  10. 1/5 global: use designated initializers for optionsPatrick Steinhardt, Apr 15, 2025
  11. 2/5 parse-options: introduce precision handling for `OPTION_INTEGER`Patrick Steinhardt, Apr 15, 2025
  12. Phillip WoodApr 15, 2025
  13. Patrick SteinhardtApr 16, 2025
  14. Junio C HamanoApr 15, 2025
  15. Patrick SteinhardtApr 16, 2025
  16. 3/5 parse-options: introduce precision handling for `OPTION_MAGNITUDE`Patrick Steinhardt, Apr 15, 2025
  17. 4/5 parse-options: introduce `OPTION_UNSIGNED`Patrick Steinhardt, Apr 15, 2025
  18. Phillip WoodApr 15, 2025
  19. Patrick SteinhardtApr 16, 2025
  20. phillip.wood123@gmail.comApr 16, 2025
  21. René ScharfeApr 15, 2025
  22. Patrick SteinhardtApr 16, 2025
  23. 5/5 parse-options: detect mismatches in integer signednessPatrick Steinhardt, Apr 15, 2025
  24. Junio C HamanoApr 15, 2025
  25. 0/7 parse-options: harden handling of integer valuesPatrick Steinhardt, Apr 16, 2025
  26. 2/7 parse-options: check for overflow when parsing integersPatrick Steinhardt, Apr 16, 2025
  27. 1/7 global: use designated initializers for optionsPatrick Steinhardt, Apr 16, 2025
  28. 4/7 parse-options: introduce precision handling for `OPTION_MAGNITUDE`Patrick Steinhardt, Apr 16, 2025
  29. 3/7 parse-options: introduce precision handling for `OPTION_INTEGER`Patrick Steinhardt, Apr 16, 2025
  30. Junio C HamanoApr 16, 2025
  31. 5/7 parse-options: introduce `OPTION_UNSIGNED`Patrick Steinhardt, Apr 16, 2025
  32. Junio C HamanoApr 16, 2025
  33. Patrick SteinhardtApr 17, 2025
  34. 7/7 parse-options: introduce bounded integer optionsPatrick Steinhardt, Apr 16, 2025
  35. Junio C HamanoApr 16, 2025
  36. Patrick SteinhardtApr 17, 2025
  37. 6/7 parse-options: detect mismatches in integer signednessPatrick Steinhardt, Apr 16, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.