* [PATCH v2 0/2] btrfs-progs: parser related cleanups
@ 2024-01-17 23:14 ` Qu Wenruo
0 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2024-01-17 23:10 UTC (permalink / raw)
To: linux-btrfs
[CHANGELOG]
v2:
- Properly return the parsed value for parse_u64_with_suffix()
Facepalm, I forgot to assign the result to @value_ret, and only relied
on cli-tests to catch them, but they are not enough to catch.
- Avoid outputting any error message inside parse_u64_with_suffix()
One error message and exit(1) call is left from previous function.
Need to be consistent with all other error situations.
- Rebased using the patch from devel branch
So that the modification David did won't need to be redone.
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 | 90 +++++++++++++++++++++++++++----------------
common/parse-utils.h | 2 +-
common/string-utils.c | 45 ++++++++++++++--------
common/string-utils.h | 9 +++++
convert/main.c | 3 +-
kernel-shared/zoned.c | 4 +-
mkfs/main.c | 6 +--
11 files changed, 118 insertions(+), 68 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] btrfs-progs: use parse_u64() to implement arg_strtou64()
@ 2024-01-17 23:14 ` Qu Wenruo
0 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2024-01-17 23:10 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
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>
Signed-off-by: David Sterba <dsterba@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..9b34f95d64df 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 a negative 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 a negative number and converts it u64, we
+ * don't really want to utilize this as the helper is meant for u64 only.
+ */
+ 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 0/2] btrfs-progs: parser related cleanups
@ 2024-01-17 23:14 ` Qu Wenruo
0 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2024-01-17 23:14 UTC (permalink / raw)
To: linux-btrfs
[CHANGELOG]
v2:
- Properly return the parsed value for parse_u64_with_suffix()
Facepalm, I forgot to assign the result to @value_ret, and only relied
on cli-tests to catch them, but they are not enough to catch.
- Avoid outputting any error message inside parse_u64_with_suffix()
One error message and exit(1) call is left from previous function.
Need to be consistent with all other error situations.
- Rebased using the patch from devel branch
So that the modification David did won't need to be redone.
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 | 90 +++++++++++++++++++++++++++----------------
common/parse-utils.h | 2 +-
common/string-utils.c | 45 ++++++++++++++--------
common/string-utils.h | 9 +++++
convert/main.c | 3 +-
kernel-shared/zoned.c | 4 +-
mkfs/main.c | 6 +--
11 files changed, 118 insertions(+), 68 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] btrfs-progs: use parse_u64() to implement arg_strtou64()
@ 2024-01-17 23:14 ` Qu Wenruo
0 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2024-01-17 23:14 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
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>
Signed-off-by: David Sterba <dsterba@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..9b34f95d64df 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 a negative 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 a negative number and converts it u64, we
+ * don't really want to utilize this as the helper is meant for u64 only.
+ */
+ 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 0/2] btrfs-progs: parser related cleanups
@ 2024-01-17 23:17 Qu Wenruo
2024-01-18 1:16 ` David Sterba
0 siblings, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2024-01-17 23:17 UTC (permalink / raw)
To: linux-btrfs
[CHANGELOG]
v2:
- Properly return the parsed value for parse_u64_with_suffix()
Facepalm, I forgot to assign the result to @value_ret, and only relied
on cli-tests to catch them, but they are not enough to catch.
- Avoid outputting any error message inside parse_u64_with_suffix()
One error message and exit(1) call is left from previous function.
Need to be consistent with all other error situations.
- Rebased using the patch from devel branch
So that the modification David did won't need to be redone.
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 | 90 +++++++++++++++++++++++++++----------------
common/parse-utils.h | 2 +-
common/string-utils.c | 45 ++++++++++++++--------
common/string-utils.h | 9 +++++
convert/main.c | 3 +-
kernel-shared/zoned.c | 4 +-
mkfs/main.c | 6 +--
11 files changed, 118 insertions(+), 68 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/2] btrfs-progs: parser related cleanups
2024-01-17 23:17 [PATCH v2 0/2] btrfs-progs: parser related cleanups Qu Wenruo
@ 2024-01-18 1:16 ` David Sterba
0 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2024-01-18 1:16 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
On Thu, Jan 18, 2024 at 09:47:00AM +1030, Qu Wenruo wrote:
> [CHANGELOG]
> v2:
> - Properly return the parsed value for parse_u64_with_suffix()
> Facepalm, I forgot to assign the result to @value_ret, and only relied
> on cli-tests to catch them, but they are not enough to catch.
>
> - Avoid outputting any error message inside parse_u64_with_suffix()
> One error message and exit(1) call is left from previous function.
> Need to be consistent with all other error situations.
>
> - Rebased using the patch from devel branch
> So that the modification David did won't need to be redone.
Updated thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-01-18 1:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-17 23:10 [PATCH v2 0/2] btrfs-progs: parser related cleanups Qu Wenruo
2024-01-17 23:14 ` Qu Wenruo
2024-01-17 23:10 ` [PATCH v2 1/2] btrfs-progs: use parse_u64() to implement arg_strtou64() Qu Wenruo
2024-01-17 23:14 ` Qu Wenruo
-- strict thread matches above, loose matches on Subject: below --
2024-01-17 23:17 [PATCH v2 0/2] btrfs-progs: parser related cleanups Qu Wenruo
2024-01-18 1:16 ` David Sterba
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.