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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Apr 01, 2025 at 08:47:12PM +0200, René Scharfe wrote:
> 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.

Oh, nice :) I'll definitely use this, thanks!

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

Agreed. We would notice quite fast in case there are any machines out
there that don't handle this well as our tests exercise this logic.

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

Some of them. We already pass both `int` and `size_t`, which means that
we definitely need at least `int32` and `int64`. Whether we also need to
handle `int8` or `int16` is a different question, but it doesn't add
much complexity anyway.

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

In most cases we probably don't need that, agreed. But there are
callsites where we use negative values to indicate the default value,
and I bet there are some options where negative values do make sense
indeed.

But in the end I think we should harden our integer handling for
options, which is where the next patch comes in that introduces
`OPTION_UNSIGNED()`. I don't want to convert all users in this patch
series, but I definitely think that we should adapt them over time so
that they stop accepting signed integers.

Patrick