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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Patrick

On 15/04/2025 13:14, Patrick Steinhardt wrote:
> 
> 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

> 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

> +
> +		}
>   
> +		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
>