* [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 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: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 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.