[PATCH 0/2] btrfs-progs: parser related cleanups

[PATCH 0/2] btrfs-progs: parser related cleanups

[Qu Wenruo]

Btrfs-progs has two types of parsers:

- parse_*()
  Those would return 0 for a good parse, and save the value into a
  pointer.
  Callers are responsible to handle the error.

- arg_strto*()
  Those would directly return the parsed value, and call exit(1)
  directly for errors.

However this split is not perfect:

- A lot of code can be shared between them
  In fact, mostly arg_strto*() can be implement using parse_*().
  The only difference is in how detailed the error string would be.

- parse_size_from_string() doesn't follow the scheme
  It follows arg_strto*() behavior but has the parse_*() name.

This patch would:

- Use parse_u64() to implement arg_strtou64()
  The first patch.

- Use parse_u64_with_suffix() to implement arg_strtou64_with_suffix()
  The new helper parse_u64_with_suffix() would replace the old
  parse_size_from_string() and do the proper error handling.

Qu Wenruo (2):
  btrfs-progs: use parse_u64() to implement arg_strtou64()
  btrfs-progs: implement arg_strtou64_with_suffix() with a new helper

 cmds/filesystem.c     | 15 ++++----
 cmds/qgroup.c         |  3 +-
 cmds/reflink.c        |  7 ++--
 cmds/scrub.c          |  2 +-
 common/parse-utils.c  | 85 ++++++++++++++++++++++++++++---------------
 common/parse-utils.h  |  2 +-
 common/string-utils.c | 45 +++++++++++++++--------
 common/string-utils.h |  3 ++
 convert/main.c        |  3 +-
 kernel-shared/zoned.c |  4 +-
 mkfs/main.c           |  6 +--
 11 files changed, 110 insertions(+), 65 deletions(-)

--
2.43.0

[PATCH 1/2] btrfs-progs: use parse_u64() to implement arg_strtou64()

[Qu Wenruo]

Both functions are just doing the same thing, the only difference is
only in error handling, as parse_u64() requires callers to handle it,
meanwhile arg_strtou64() would call exit(1).

This patch would convert arg_strtou64() to utilize parse_u64(), and use
the return value to output different error messages.

This also means the return value of parse_u64() would be more than just
0 or 1, but -EINVAL for invalid string (including no numeric string at
all, has any tailing characters, or minus value), and -ERANGE for
overflow.

The existing callers are only checking if the return value is 0, thus
not really affected.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 common/parse-utils.c  | 19 ++++++++++++++++++-
 common/string-utils.c | 26 ++++++++++----------------
 common/string-utils.h |  2 ++
 3 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/common/parse-utils.c b/common/parse-utils.c
index 3d9a6d637a7f..e2b8687d53d7 100644
--- a/common/parse-utils.c
+++ b/common/parse-utils.c
@@ -31,14 +31,31 @@
 #include "common/messages.h"
 #include "common/utils.h"
 
+/*
+ * Parse a string to u64.
+ *
+ * Return 0 if there is a valid numeric string and result would be stored in
+ * @result.
+ * Return -EINVAL if the string is not valid (no numeric string at all, or
+ * has any tailing characters, or minus value).
+ * Return -ERANGE if the value is too larger for u64.
+ */
 int parse_u64(const char *str, u64 *result)
 {
 	char *endptr;
 	u64 val;
 
+	/*
+	 * Although strtoull accepts minus number and convert it u64,
+	 * we don't really want to utilize this behavior.
+	 */
+	if (str[0] == '-')
+		return -EINVAL;
 	val = strtoull(str, &endptr, 10);
 	if (*endptr)
-		return 1;
+		return -EINVAL;
+	if (val == ULLONG_MAX && errno == ERANGE)
+		return -ERANGE;
 
 	*result = val;
 	return 0;
diff --git a/common/string-utils.c b/common/string-utils.c
index e338afa75711..ba5cc55a6daa 100644
--- a/common/string-utils.c
+++ b/common/string-utils.c
@@ -20,6 +20,7 @@
 #include <limits.h>
 #include "common/string-utils.h"
 #include "common/messages.h"
+#include "common/parse-utils.h"
 
 int string_is_numerical(const char *str)
 {
@@ -50,25 +51,18 @@ int string_has_prefix(const char *str, const char *prefix)
 u64 arg_strtou64(const char *str)
 {
 	u64 value;
-	char *ptr_parse_end = NULL;
+	int ret;
 
-	value = strtoull(str, &ptr_parse_end, 0);
-	if (ptr_parse_end && *ptr_parse_end != '\0') {
-		error("%s is not a valid numeric value", str);
-		exit(1);
-	}
-
-	/*
-	 * if we pass a negative number to strtoull, it will return an
-	 * unexpected number to us, so let's do the check ourselves.
-	 */
-	if (str[0] == '-') {
-		error("%s: negative value is invalid", str);
-		exit(1);
-	}
-	if (value == ULLONG_MAX) {
+	ret = parse_u64(str, &value);
+	if (ret == -ERANGE) {
 		error("%s is too large", str);
 		exit(1);
+	} else if (ret == -EINVAL) {
+		if (str[0] == '-')
+			error("%s: negative value is invalid", str);
+		else
+			error("%s is not a valid numeric value", str);
+		exit(1);
 	}
 	return value;
 }
diff --git a/common/string-utils.h b/common/string-utils.h
index ade8fcd1f66b..a7ac8f5c7efa 100644
--- a/common/string-utils.h
+++ b/common/string-utils.h
@@ -17,6 +17,8 @@
 #ifndef __BTRFS_STRING_UTILS_H__
 #define __BTRFS_STRING_UTILS_H__
 
+#include "kerncompat.h"
+
 int string_is_numerical(const char *str);
 int string_has_prefix(const char *str, const char *prefix);
 u64 arg_strtou64(const char *str);
-- 
2.43.0

[PATCH 2/2] btrfs-progs: implement arg_strtou64_with_suffix() with a new helper

[Qu Wenruo]

This patch would introduce a new parser helper, parse_u64_with_suffix(),
which would have better error handling, following all the parse_*()
helpers to return non-zero value for errors.

This new helper is going to replace parse_size_from_string(), which
would directly call exit(1) to stop the whole program.

Furthermore most callers of parse_size_from_string() are expecting
exit(1) for error, so that they can skip the error handling.

For those call sites, introduce a wrapper, arg_strtou64_with_suffix(),
to do that.
The only disadvantage is a little less detailed error report for why the
parse failed, but for most cases the generic error string should be
enough.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/filesystem.c     | 15 +++++-----
 cmds/qgroup.c         |  3 +-
 cmds/reflink.c        |  7 +++--
 cmds/scrub.c          |  2 +-
 common/parse-utils.c  | 66 ++++++++++++++++++++++++-------------------
 common/parse-utils.h  |  2 +-
 common/string-utils.c | 19 +++++++++++++
 common/string-utils.h |  1 +
 convert/main.c        |  3 +-
 kernel-shared/zoned.c |  4 +--
 mkfs/main.c           |  6 ++--
 11 files changed, 80 insertions(+), 48 deletions(-)

diff --git a/cmds/filesystem.c b/cmds/filesystem.c
index 1b444b8f6c07..e0e74fbe6d6e 100644
--- a/cmds/filesystem.c
+++ b/cmds/filesystem.c
@@ -53,6 +53,7 @@
 #include "common/device-utils.h"
 #include "common/open-utils.h"
 #include "common/parse-utils.h"
+#include "common/string-utils.h"
 #include "common/filesystem-utils.h"
 #include "common/format-output.h"
 #include "cmds/commands.h"
@@ -1065,13 +1066,13 @@ static int cmd_filesystem_defrag(const struct cmd_struct *cmd,
 				bconf_be_verbose();
 			break;
 		case 's':
-			start = parse_size_from_string(optarg);
+			start = arg_strtou64_with_suffix(optarg);
 			break;
 		case 'l':
-			len = parse_size_from_string(optarg);
+			len = arg_strtou64_with_suffix(optarg);
 			break;
 		case 't':
-			thresh = parse_size_from_string(optarg);
+			thresh = arg_strtou64_with_suffix(optarg);
 			if (thresh > (u32)-1) {
 				warning(
 			    "target extent size %llu too big, trimmed to %u",
@@ -1083,7 +1084,7 @@ static int cmd_filesystem_defrag(const struct cmd_struct *cmd,
 			recursive = true;
 			break;
 		case GETOPT_VAL_STEP:
-			defrag_global_step = parse_size_from_string(optarg);
+			defrag_global_step = arg_strtou64_with_suffix(optarg);
 			if (defrag_global_step < SZ_256K) {
 				warning("step %llu too small, adjusting to 256KiB\n",
 					   defrag_global_step);
@@ -1312,8 +1313,8 @@ static int check_resize_args(const char *amount, const char *path, u64 *devid_re
 			mod = 1;
 			sizestr++;
 		}
-		diff = parse_size_from_string(sizestr);
-		if (!diff) {
+		ret = parse_u64_with_suffix(sizestr, &diff);
+		if (ret < 0) {
 			error("failed to parse size %s", sizestr);
 			ret = 1;
 			goto out;
@@ -1605,7 +1606,7 @@ static int cmd_filesystem_mkswapfile(const struct cmd_struct *cmd, int argc, cha
 
 		switch (c) {
 		case 's':
-			size = parse_size_from_string(optarg);
+			size = arg_strtou64_with_suffix(optarg);
 			/* Minimum limit reported by mkswap */
 			if (size < 40 * SZ_1K) {
 				error("swapfile needs to be at least 40 KiB");
diff --git a/cmds/qgroup.c b/cmds/qgroup.c
index 2f1893cbe6e5..68791428b25f 100644
--- a/cmds/qgroup.c
+++ b/cmds/qgroup.c
@@ -40,6 +40,7 @@
 #include "common/help.h"
 #include "common/units.h"
 #include "common/parse-utils.h"
+#include "common/string-utils.h"
 #include "common/format-output.h"
 #include "common/messages.h"
 #include "cmds/commands.h"
@@ -2114,7 +2115,7 @@ static int cmd_qgroup_limit(const struct cmd_struct *cmd, int argc, char **argv)
 	if (!strcasecmp(argv[optind], "none"))
 		size = -1ULL;
 	else
-		size = parse_size_from_string(argv[optind]);
+		size = arg_strtou64_with_suffix(argv[optind]);
 
 	memset(&args, 0, sizeof(args));
 	if (compressed)
diff --git a/cmds/reflink.c b/cmds/reflink.c
index 867a5bd2ac0a..669a5fc10e5d 100644
--- a/cmds/reflink.c
+++ b/cmds/reflink.c
@@ -25,6 +25,7 @@
 #include "common/messages.h"
 #include "common/open-utils.h"
 #include "common/parse-utils.h"
+#include "common/string-utils.h"
 #include "common/help.h"
 #include "cmds/commands.h"
 
@@ -76,7 +77,7 @@ static void parse_reflink_range(const char *str, u64 *from, u64 *length, u64 *to
 		error("wrong range spec near %s", str);
 		exit(1);
 	}
-	*from = parse_size_from_string(tmp);
+	*from = arg_strtou64_with_suffix(tmp);
 	str++;
 
 	/* Parse length */
@@ -91,11 +92,11 @@ static void parse_reflink_range(const char *str, u64 *from, u64 *length, u64 *to
 		error("wrong range spec near %s", str);
 		exit(1);
 	}
-	*length = parse_size_from_string(tmp);
+	*length =arg_strtou64_with_suffix(tmp);
 	str++;
 
 	/* Parse to, until end of string */
-	*to = parse_size_from_string(str);
+	*to = arg_strtou64_with_suffix(str);
 }
 
 static int reflink_apply_range(int fd_in, int fd_out, const struct reflink_range *range)
diff --git a/cmds/scrub.c b/cmds/scrub.c
index f4c963d705bd..e105ecb2fa5d 100644
--- a/cmds/scrub.c
+++ b/cmds/scrub.c
@@ -2035,7 +2035,7 @@ static int cmd_scrub_limit(const struct cmd_struct *cmd, int argc, char **argv)
 			devid_set = true;
 			break;
 		case 'l':
-			opt_limit = parse_size_from_string(optarg);
+			opt_limit = arg_strtou64_with_suffix(optarg);
 			limit_set = true;
 			break;
 		default:
diff --git a/common/parse-utils.c b/common/parse-utils.c
index e2b8687d53d7..1b8b950c89aa 100644
--- a/common/parse-utils.c
+++ b/common/parse-utils.c
@@ -160,40 +160,45 @@ int parse_range_strict(const char *range, u64 *start, u64 *end)
 	return 1;
 }
 
-u64 parse_size_from_string(const char *s)
+/*
+ * Parse a string to u64, with support for suffixes.
+ *
+ * The suffixes are all 1024 based, and is case-insensitive.
+ * The supported ones are "KMGPTE", with one extra suffix "B" supported.
+ * "B" just means byte, thus won't change the value.
+ *
+ * After one or less suffix, there should be no extra character other than
+ * a tailing NUL.
+ *
+ * Return 0 if there is a valid numeric string and result would be stored in
+ * @result.
+ * Return -EINVAL if the string is not valid (no numeric string at all,
+ * has any tailing characters, or minus value).
+ * Return -ERANGE if the value is too larger for u64.
+ */
+int parse_u64_with_suffix(const char *s, u64 *value_ret)
 {
 	char c;
 	char *endptr;
 	u64 mult = 1;
-	u64 ret;
+	u64 value;
+
+	if (!s)
+		return -EINVAL;
+	if (s[0] == '-')
+		return -EINVAL;
 
-	if (!s) {
-		error("size value is empty");
-		exit(1);
-	}
-	if (s[0] == '-') {
-		error("size value '%s' is less equal than 0", s);
-		exit(1);
-	}
 	errno = 0;
-	ret = strtoull(s, &endptr, 10);
-	if (endptr == s) {
-		error("size value '%s' is invalid", s);
-		exit(1);
-	}
-	if (endptr[0] && endptr[1]) {
-		error("illegal suffix contains character '%c' in wrong position",
-			endptr[1]);
-		exit(1);
-	}
+	value = strtoull(s, &endptr, 10);
+	if (endptr == s)
+		return -EINVAL;
+
 	/*
 	 * strtoll returns LLONG_MAX when overflow, if this happens,
 	 * need to call strtoull to get the real size
 	 */
-	if (errno == ERANGE && ret == ULLONG_MAX) {
-		error("size value '%s' is too large for u64", s);
-		exit(1);
-	}
+	if (errno == ERANGE && value == ULLONG_MAX)
+		return -ERANGE;
 	if (endptr[0]) {
 		c = tolower(endptr[0]);
 		switch (c) {
@@ -218,17 +223,20 @@ u64 parse_size_from_string(const char *s)
 		case 'b':
 			break;
 		default:
-			error("unknown size descriptor '%c'", c);
-			exit(1);
+			return -EINVAL;
 		}
+		endptr++;
 	}
+	/* Tailing character check. */
+	if (endptr[0] != '\0')
+		return -EINVAL;
 	/* Check whether ret * mult overflow */
-	if (fls64(ret) + fls64(mult) - 1 > 64) {
+	if (fls64(value) + fls64(mult) - 1 > 64) {
 		error("size value '%s' is too large for u64", s);
 		exit(1);
 	}
-	ret *= mult;
-	return ret;
+	value *= mult;
+	return 0;
 }
 
 enum btrfs_csum_type parse_csum_type(const char *s)
diff --git a/common/parse-utils.h b/common/parse-utils.h
index 47f202c64f03..33ff9ca14346 100644
--- a/common/parse-utils.h
+++ b/common/parse-utils.h
@@ -19,10 +19,10 @@
 
 #include "kerncompat.h"
 
-u64 parse_size_from_string(const char *s);
 enum btrfs_csum_type parse_csum_type(const char *s);
 
 int parse_u64(const char *str, u64 *result);
+int parse_u64_with_suffix(const char *s, u64 *value_ret);
 int parse_range_u32(const char *range, u32 *start, u32 *end);
 int parse_range(const char *range, u64 *start, u64 *end);
 int parse_range_strict(const char *range, u64 *start, u64 *end);
diff --git a/common/string-utils.c b/common/string-utils.c
index ba5cc55a6daa..5bd72fc5249e 100644
--- a/common/string-utils.c
+++ b/common/string-utils.c
@@ -67,3 +67,22 @@ u64 arg_strtou64(const char *str)
 	return value;
 }
 
+u64 arg_strtou64_with_suffix(const char *str)
+{
+	u64 value;
+	int ret;
+
+	ret = parse_u64_with_suffix(str, &value);
+	if (ret == -ERANGE) {
+		error("%s is too large", str);
+		exit(1);
+	} else if (ret == -EINVAL) {
+		error("%s is not a valid numeric value with supported size suffixes",
+		      str);
+		exit(1);
+	} else if (ret < 0) {
+		errno = -ret;
+		error("failed to parse string '%s': %m", str);
+	}
+	return value;
+}
diff --git a/common/string-utils.h b/common/string-utils.h
index a7ac8f5c7efa..38137d566dbc 100644
--- a/common/string-utils.h
+++ b/common/string-utils.h
@@ -22,5 +22,6 @@
 int string_is_numerical(const char *str);
 int string_has_prefix(const char *str, const char *prefix);
 u64 arg_strtou64(const char *str);
+u64 arg_strtou64_with_suffix(const char *str);
 
 #endif
diff --git a/convert/main.c b/convert/main.c
index 77b7c0516ae5..f18fab4a236c 100644
--- a/convert/main.c
+++ b/convert/main.c
@@ -114,6 +114,7 @@
 #include "common/path-utils.h"
 #include "common/help.h"
 #include "common/parse-utils.h"
+#include "common/string-utils.h"
 #include "common/fsfeatures.h"
 #include "common/device-scan.h"
 #include "common/box.h"
@@ -1925,7 +1926,7 @@ int BOX_MAIN(convert)(int argc, char *argv[])
 				packing = 0;
 				break;
 			case 'N':
-				nodesize = parse_size_from_string(optarg);
+				nodesize = arg_strtou64_with_suffix(optarg);
 				break;
 			case 'r':
 				rollback = 1;
diff --git a/kernel-shared/zoned.c b/kernel-shared/zoned.c
index 9c40e4ef6c17..fb1e1388804e 100644
--- a/kernel-shared/zoned.c
+++ b/kernel-shared/zoned.c
@@ -34,7 +34,7 @@
 #include "common/device-utils.h"
 #include "common/extent-cache.h"
 #include "common/internal.h"
-#include "common/parse-utils.h"
+#include "common/string-utils.h"
 #include "common/messages.h"
 #include "mkfs/common.h"
 
@@ -102,7 +102,7 @@ u64 zone_size(const char *file)
 
 		tmp = bconf_param_value("zone-size");
 		if (tmp) {
-			size = parse_size_from_string(tmp);
+			size = arg_strtou64_with_suffix(tmp);
 			if (!is_power_of_2(size) || size < BTRFS_MIN_ZONE_SIZE ||
 			    size > BTRFS_MAX_ZONE_SIZE) {
 				error("invalid emulated zone size %llu", size);
diff --git a/mkfs/main.c b/mkfs/main.c
index d984c9955859..b9882208dbd5 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -1264,7 +1264,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 				ret = 1;
 				goto error;
 			case 'n':
-				nodesize = parse_size_from_string(optarg);
+				nodesize = arg_strtou64_with_suffix(optarg);
 				nodesize_forced = true;
 				break;
 			case 'L':
@@ -1329,10 +1329,10 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 				break;
 				}
 			case 's':
-				sectorsize = parse_size_from_string(optarg);
+				sectorsize = arg_strtou64_with_suffix(optarg);
 				break;
 			case 'b':
-				block_count = parse_size_from_string(optarg);
+				block_count = arg_strtou64_with_suffix(optarg);
 				opt_zero_end = false;
 				break;
 			case 'v':
-- 
2.43.0

Re: [PATCH 0/2] btrfs-progs: parser related cleanups

[David Sterba]

On Wed, Jan 17, 2024 at 02:40:38PM +1030, Qu Wenruo wrote:
> Btrfs-progs has two types of parsers:
> 
> - parse_*()
>   Those would return 0 for a good parse, and save the value into a
>   pointer.
>   Callers are responsible to handle the error.
> 
> - arg_strto*()
>   Those would directly return the parsed value, and call exit(1)
>   directly for errors.
> 
> However this split is not perfect:
> 
> - A lot of code can be shared between them
>   In fact, mostly arg_strto*() can be implement using parse_*().
>   The only difference is in how detailed the error string would be.
> 
> - parse_size_from_string() doesn't follow the scheme
>   It follows arg_strto*() behavior but has the parse_*() name.
> 
> This patch would:
> 
> - Use parse_u64() to implement arg_strtou64()
>   The first patch.
> 
> - Use parse_u64_with_suffix() to implement arg_strtou64_with_suffix()
>   The new helper parse_u64_with_suffix() would replace the old
>   parse_size_from_string() and do the proper error handling.
> 
> Qu Wenruo (2):
>   btrfs-progs: use parse_u64() to implement arg_strtou64()
>   btrfs-progs: implement arg_strtou64_with_suffix() with a new helper

Thanks, that's better than I expected, it's clear what helpers can be
used where just by the name. I did some minor adjustments or added a
comment for the arg_* helper declarations.