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

["René Scharfe" <l.s.r@web.de>]

Am 01.04.25 um 17:01 schrieb Patrick Steinhardt:
> diff --git a/parse-options.c b/parse-options.c
> index 35fbb3b0d63..dbda9b7cfe7 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -172,25 +172,50 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
>  			return (*opt->ll_callback)(p, opt, p_arg, p_unset);
>  	}
>  	case OPTION_INTEGER:
> +	{
> +		intmax_t upper_bound = (((intmax_t) 1 << (opt->precision * 8 - 1)) - 1);

Ugh, how does this not overflow?  The macro maximum_signed_value_of_type
does a similar calculation better.

> +		intmax_t lower_bound = -upper_bound - 1;

This depends on two's complement being used, which is bad for purity and
portability to obsolete machines, but probably OK in practice.

> +		intmax_t value;
> +
>  		if (unset) {
> -			*(int *)opt->value = 0;
> -			return 0;
> -		}
> -		if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
> -			*(int *)opt->value = opt->defval;
> -			return 0;
> -		}
> -		if (get_arg(p, opt, flags, &arg))
> +			value = 0;
> +		} else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
> +			value = opt->defval;
> +		} else if (get_arg(p, opt, flags, &arg)) {
>  			return -1;
> -		if (!*arg)
> +		} else if (!*arg) {
>  			return error(_("%s expects a numerical value"),
>  				     optname(opt, flags));
> -		*(int *)opt->value = strtol(arg, (char **)&s, 10);
> -		if (*s)
> -			return error(_("%s expects a numerical value"),
> -				     optname(opt, flags));
> -		return 0;
> +		} else {
> +			value = strtoimax(arg, (char **)&s, 10);
> +			if (*s)
> +				return error(_("%s expects a numerical value"),
> +					     optname(opt, flags));
> +
> +		}
>
> +		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;

Do we even need all these sizes?  Or can we whittle it down to ssize_t?

And for which quantities do we need to accept negative values anyway?

René