From: Phillip Wood Date: Tue, 15 Apr 2025 15:51:59 GMT Subject: Re: [PATCH v2 2/5] parse-options: introduce precision handling for `OPTION_INTEGER` 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: > > 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 > > -i, --[no-]integer > get a integer > + --[no-]i16 get a 16 bit integer > -j get a integer, too > -m, --magnitude 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 >