diff mbox series

[v3,5/7] parse-options: introduce `OPTION_UNSIGNED`

Message ID 20250416-b4-pks-parse-options-integers-v3-5-d390746bea79@pks.im (mailing list archive)
State Superseded
Headers show
Series parse-options: harden handling of integer values | expand

Commit Message

Patrick Steinhardt April 16, 2025, 10:02 a.m. UTC
We have two generic ways to parse integers in the "parse-options"
subsystem:

  - `OPTION_INTEGER` parses a signed integer.

  - `OPTION_MAGNITUDE` parses an unsigned integer, but it also
    interprets suffixes like "k" or "g".

Notably missing is a middle ground that parses unsigned integers without
interpreting suffixes. Introduce a new `OPTION_UNSIGNED` option type to
plug this gap. This option type will be used in subsequent commits.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 parse-options.c               | 54 +++++++++++++++++++++++++++++++++++++++++++
 parse-options.h               | 12 ++++++++++
 t/helper/test-parse-options.c |  4 +++-
 t/t0040-parse-options.sh      | 24 ++++++++++++++++++-
 4 files changed, 92 insertions(+), 2 deletions(-)

Comments

Junio C Hamano April 16, 2025, 6:50 p.m. UTC | #1
Patrick Steinhardt <ps@pks.im> writes:

> We have two generic ways to parse integers in the "parse-options"
> subsystem:
>
>   - `OPTION_INTEGER` parses a signed integer.
>
>   - `OPTION_MAGNITUDE` parses an unsigned integer, but it also
>     interprets suffixes like "k" or "g".
>
> Notably missing is a middle ground that parses unsigned integers without
> interpreting suffixes. Introduce a new `OPTION_UNSIGNED` option type to
> plug this gap. This option type will be used in subsequent commits.

This takes a turn in a bit unexpected direction.  Because the way to
spell a scaled quantity is unambiguous in the sense that anything
that used to be parsed with OPT_INTEGER() couldn't have had anything
but "^-?[0-9]+$", an obvious alternative is to teach OPTION_INTEGER
to always allow the scaling suffix if the user wants to use one,
without adding a new "only numbers but this one does not even allow
a sign" variant.

Seriously, are there good candidates for an option where we want to
absolutely refuse to take scaling suffix and insist only on a bare
number?
Patrick Steinhardt April 17, 2025, 8:15 a.m. UTC | #2
On Wed, Apr 16, 2025 at 11:50:42AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > We have two generic ways to parse integers in the "parse-options"
> > subsystem:
> >
> >   - `OPTION_INTEGER` parses a signed integer.
> >
> >   - `OPTION_MAGNITUDE` parses an unsigned integer, but it also
> >     interprets suffixes like "k" or "g".
> >
> > Notably missing is a middle ground that parses unsigned integers without
> > interpreting suffixes. Introduce a new `OPTION_UNSIGNED` option type to
> > plug this gap. This option type will be used in subsequent commits.
> 
> This takes a turn in a bit unexpected direction.  Because the way to
> spell a scaled quantity is unambiguous in the sense that anything
> that used to be parsed with OPT_INTEGER() couldn't have had anything
> but "^-?[0-9]+$", an obvious alternative is to teach OPTION_INTEGER
> to always allow the scaling suffix if the user wants to use one,
> without adding a new "only numbers but this one does not even allow
> a sign" variant.

Yes, that would be the alternative indeed. I wanted to have this patch
so that I can adapt callsites that accidentally pass unsigned integers
to the signed option without introducing any kind of incompatibility. So
I was mostly aiming for 1:1 compatibility.

But I'm equally fine with extending `OPT_INTEGER()` to support parsing
magnitudes. I guess it's even preferable: less overhead on our side, and
more consistency for our users.

> Seriously, are there good candidates for an option where we want to
> absolutely refuse to take scaling suffix and insist only on a bare
> number?

I cannot think of any specific reason.

Patrick
diff mbox series

Patch

diff --git a/parse-options.c b/parse-options.c
index 259716efb17..e4dc22464b2 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -222,6 +222,60 @@  static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 			    optname(opt, flags));
 		}
 	}
+	case OPTION_UNSIGNED:
+	{
+		uintmax_t upper_bound = UINTMAX_MAX >> (bitsizeof(uintmax_t) - CHAR_BIT * opt->precision);
+		uintmax_t value;
+
+		if (unset) {
+			value = 0;
+		} else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
+			value = opt->defval;
+		} else if (get_arg(p, opt, flags, &arg)) {
+			return -1;
+		} else if (!*arg) {
+			return error(_("%s expects a numerical value"),
+				     optname(opt, flags));
+		} else if (*arg == '-') {
+			return error(_("%s does not accept negative values"),
+				     optname(opt, flags));
+		} else {
+			errno = 0;
+			value = strtoumax(arg, (char **)&s, 10);
+			if (*s)
+				return error(_("%s expects a numerical value"),
+					     optname(opt, flags));
+			if (errno == ERANGE)
+				return error(_("value %s for %s not in range [%"PRIuMAX",%"PRIuMAX"]"),
+					     arg, optname(opt, flags), (uintmax_t)0, (uintmax_t)upper_bound);
+			if (errno)
+				return error_errno(_("value %s for %s cannot be parsed"),
+						   arg, optname(opt, flags));
+
+		}
+
+		if (value > upper_bound)
+			return error(_("value %s for %s not in range [%"PRIuMAX",%"PRIuMAX"]"),
+				     arg, optname(opt, flags), (uintmax_t)0, (uintmax_t)upper_bound);
+
+		switch (opt->precision) {
+		case 1:
+			*(uint8_t *)opt->value = value;
+			return 0;
+		case 2:
+			*(uint16_t *)opt->value = value;
+			return 0;
+		case 4:
+			*(uint32_t *)opt->value = value;
+			return 0;
+		case 8:
+			*(uint64_t *)opt->value = value;
+			return 0;
+		default:
+			BUG("invalid precision for option %s",
+			    optname(opt, flags));
+		}
+	}
 	case OPTION_MAGNITUDE:
 	{
 		uintmax_t upper_bound = UINTMAX_MAX >> (bitsizeof(uintmax_t) - CHAR_BIT * opt->precision);
diff --git a/parse-options.h b/parse-options.h
index 55c42faa29f..aa37134dc72 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -25,6 +25,7 @@  enum parse_opt_type {
 	/* options with arguments (usually) */
 	OPTION_STRING,
 	OPTION_INTEGER,
+	OPTION_UNSIGNED,
 	OPTION_MAGNITUDE,
 	OPTION_CALLBACK,
 	OPTION_LOWLEVEL_CALLBACK,
@@ -224,6 +225,16 @@  struct option {
 	.help = (h), \
 	.flags = (f), \
 }
+#define OPT_UNSIGNED_F(s, l, v, h, f) { \
+	.type = OPTION_UNSIGNED, \
+	.short_name = (s), \
+	.long_name = (l), \
+	.value = (v), \
+	.precision = sizeof(*v), \
+	.argh = N_("n"), \
+	.help = (h), \
+	.flags = (f), \
+}
 
 #define OPT_END() { \
 	.type = OPTION_END, \
@@ -276,6 +287,7 @@  struct option {
 #define OPT_CMDMODE(s, l, v, h, i)  OPT_CMDMODE_F(s, l, v, h, i, 0)
 
 #define OPT_INTEGER(s, l, v, h)     OPT_INTEGER_F(s, l, v, h, 0)
+#define OPT_UNSIGNED(s, l, v, h)    OPT_UNSIGNED_F(s, l, v, h, 0)
 #define OPT_MAGNITUDE(s, l, v, h) { \
 	.type = OPTION_MAGNITUDE, \
 	.short_name = (s), \
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index 46deb4317ef..0d559288d9c 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -120,7 +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;
-	uint16_t m16 = 0;
+	uint16_t m16 = 0, u16 = 0;
 	int16_t i16 = 0;
 
 	struct option options[] = {
@@ -142,6 +142,7 @@  int cmd__parse_options(int argc, const char **argv)
 		OPT_GROUP(""),
 		OPT_INTEGER('i', "integer", &integer, "get a integer"),
 		OPT_INTEGER(0, "i16", &i16, "get a 16 bit integer"),
+		OPT_UNSIGNED(0, "u16", &u16, "get a 16 bit unsigned integer"),
 		OPT_INTEGER('j', NULL, &integer, "get a integer, too"),
 		OPT_MAGNITUDE('m', "magnitude", &magnitude, "get a magnitude"),
 		OPT_MAGNITUDE(0, "m16", &m16, "get a 16 bit magnitude"),
@@ -215,6 +216,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, "u16: %"PRIuMAX, (uintmax_t) u16);
 	show(&expect, &ret, "magnitude: %lu", magnitude);
 	show(&expect, &ret, "m16: %"PRIuMAX, (uintmax_t) m16);
 	show(&expect, &ret, "timestamp: %"PRItime, timestamp);
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 8daaf568485..66875ce0586 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -23,6 +23,7 @@  usage: test-tool parse-options <options>
     -i, --[no-]integer <n>
                           get a integer
     --[no-]i16 <n>        get a 16 bit integer
+    --[no-]u16 <n>        get a 16 bit unsigned integer
     -j <n>                get a integer, too
     -m, --magnitude <n>   get a magnitude
     --m16 <n>             get a 16 bit magnitude
@@ -139,6 +140,7 @@  cat >expect <<\EOF
 boolean: 2
 integer: 1729
 i16: 0
+u16: 0
 magnitude: 16384
 m16: 0
 timestamp: 0
@@ -161,6 +163,7 @@  cat >expect <<\EOF
 boolean: 2
 integer: 1729
 i16: 9000
+u16: 5432
 magnitude: 16384
 m16: 32768
 timestamp: 0
@@ -173,7 +176,7 @@  file: prefix/fi.le
 EOF
 
 test_expect_success 'long options' '
-	test-tool parse-options --boolean --integer 1729 --i16 9000 --magnitude 16k \
+	test-tool parse-options --boolean --integer 1729 --i16 9000 --u16 5432 --magnitude 16k \
 		--m16 32k --boolean --string2=321 --verbose --verbose --no-dry-run \
 		--abbrev=10 --file fi.le --obsolete \
 		>output 2>output.err &&
@@ -186,6 +189,7 @@  test_expect_success 'abbreviate to something longer than SHA1 length' '
 	boolean: 0
 	integer: 0
 	i16: 0
+	u16: 0
 	magnitude: 0
 	m16: 0
 	timestamp: 0
@@ -262,6 +266,7 @@  cat >expect <<\EOF
 boolean: 1
 integer: 13
 i16: 0
+u16: 0
 magnitude: 0
 m16: 0
 timestamp: 0
@@ -287,6 +292,7 @@  cat >expect <<\EOF
 boolean: 0
 integer: 2
 i16: 0
+u16: 0
 magnitude: 0
 m16: 0
 timestamp: 0
@@ -356,6 +362,7 @@  Callback: "four", 0
 boolean: 5
 integer: 4
 i16: 0
+u16: 0
 magnitude: 0
 m16: 0
 timestamp: 0
@@ -383,6 +390,7 @@  cat >expect <<\EOF
 boolean: 1
 integer: 23
 i16: 0
+u16: 0
 magnitude: 0
 m16: 0
 timestamp: 0
@@ -464,6 +472,7 @@  cat >expect <<\EOF
 boolean: 0
 integer: 0
 i16: 0
+u16: 0
 magnitude: 0
 m16: 0
 timestamp: 0
@@ -826,4 +835,17 @@  test_expect_success 'm16 limits range' '
 	test_grep "value 65536 for option .m16. not in range \[0,65535\]" err
 '
 
+test_expect_success 'u16 limits range' '
+	test-tool parse-options --u16 65535 >out &&
+	test_grep "u16: 65535" out &&
+	test_must_fail test-tool parse-options --u16 65536 2>err &&
+	test_grep "value 65536 for option .u16. not in range \[0,65535\]" err
+'
+
+test_expect_success 'u16 does not accept negative value' '
+	test_must_fail test-tool parse-options --u16 -1 >out 2>err &&
+	test_grep "option .u16. does not accept negative values" err &&
+	test_must_be_empty out
+'
+
 test_done