* [PATCH] btrfs-progs: Improve the parse_size() error message.
@ 2014-05-20 7:51 Qu Wenruo
2014-05-28 17:20 ` David Sterba
2014-05-28 21:07 ` Mike Fleetwood
0 siblings, 2 replies; 5+ messages in thread
From: Qu Wenruo @ 2014-05-20 7:51 UTC (permalink / raw)
To: linux-btrfs
When using parse_size(), even non-numeric value is passed, it will only
give error message "ERROR: size value is empty", which is quite
confusing for end users.
This patch will introduce more meaningful error message for the
following new cases
1) Invalid size string (non-numeric string)
2) Minus size value (like "-1K")
Also this patch will take full use of endptr returned by strtoll() to
reduce unneeded loop.
Reported-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
utils.c | 53 +++++++++++++++++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 20 deletions(-)
diff --git a/utils.c b/utils.c
index 392c5cf..1cbe102 100644
--- a/utils.c
+++ b/utils.c
@@ -1612,18 +1612,20 @@ scan_again:
u64 parse_size(char *s)
{
- int i;
char c;
u64 mult = 1;
-
- for (i = 0; s && s[i] && isdigit(s[i]); i++) ;
- if (!i) {
- fprintf(stderr, "ERROR: size value is empty\n");
- exit(50);
- }
-
- if (s[i]) {
- c = tolower(s[i]);
+ long long int ret = 0;
+ char *endptr;
+
+ if (!s)
+ goto empty;
+ ret = strtoll(s, &endptr, 10);
+ if (endptr == s)
+ goto invalid;
+ if (endptr[0] && endptr[1])
+ goto suffix;
+ if (endptr[0]) {
+ c = tolower(endptr[0]);
switch (c) {
case 'e':
mult *= 1024;
@@ -1646,18 +1648,29 @@ u64 parse_size(char *s)
case 'b':
break;
default:
- fprintf(stderr, "ERROR: Unknown size descriptor "
- "'%c'\n", c);
- exit(1);
+ goto descriptor;
}
}
- if (s[i] && s[i+1]) {
- fprintf(stderr, "ERROR: Illegal suffix contains "
- "character '%c' in wrong position\n",
- s[i+1]);
- exit(51);
- }
- return strtoull(s, NULL, 10) * mult;
+ ret *= mult;
+ if (ret <= 0)
+ goto minus;
+ return (u64) ret;
+empty:
+ fprintf(stderr, "ERROR: Size value is empty\n");
+ exit(50);
+suffix:
+ fprintf(stderr, "ERROR: Illegal suffix contains character '%c' in wrong position\n",
+ endptr[1]);
+ exit(51);
+descriptor:
+ fprintf(stderr, "ERROR: Unknown size descriptor '%c'\n", c);
+ exit(52);
+invalid:
+ fprintf(stderr, "ERROR: Size value '%s' is invalid\n", s);
+ exit(53);
+minus:
+ fprintf(stderr, "ERROR: Size value '%s' is less equal than 0\n", s);
+ exit(54);
}
int open_file_or_dir3(const char *fname, DIR **dirstream, int open_flags)
--
1.9.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs-progs: Improve the parse_size() error message.
2014-05-20 7:51 [PATCH] btrfs-progs: Improve the parse_size() error message Qu Wenruo
@ 2014-05-28 17:20 ` David Sterba
2014-05-29 0:41 ` Qu Wenruo
2014-05-28 21:07 ` Mike Fleetwood
1 sibling, 1 reply; 5+ messages in thread
From: David Sterba @ 2014-05-28 17:20 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
On Tue, May 20, 2014 at 03:51:45PM +0800, Qu Wenruo wrote:
> When using parse_size(), even non-numeric value is passed, it will only
> give error message "ERROR: size value is empty", which is quite
> confusing for end users.
>
> This patch will introduce more meaningful error message for the
> following new cases
> 1) Invalid size string (non-numeric string)
> 2) Minus size value (like "-1K")
That's great. A few things below
> u64 parse_size(char *s)
> {
> - int i;
> char c;
> u64 mult = 1;
> -
> - for (i = 0; s && s[i] && isdigit(s[i]); i++) ;
> - if (!i) {
> - fprintf(stderr, "ERROR: size value is empty\n");
> - exit(50);
> - }
> -
> - if (s[i]) {
> - c = tolower(s[i]);
> + long long int ret = 0;
> + char *endptr;
> +
> + if (!s)
> + goto empty;
> + ret = strtoll(s, &endptr, 10);
> - return strtoull(s, NULL, 10) * mult;
Now it does not parse sizes from the range LLONG_MAX to ULLONG_MAX (8EiB
to 16EiB). Though we don't have problems with such sizes in real life,
I'd rather keep strtoull so it matches the advertised maximum filesystem
size.
Negative numbers can be simply identified by leading '-'. The only case
where negative number is allowed is in 'fi resize', but the string is
passed to kernel as-is.
> +empty:
> + fprintf(stderr, "ERROR: Size value is empty\n");
> + exit(50);
> +suffix:
> + fprintf(stderr, "ERROR: Illegal suffix contains character '%c' in wrong position\n",
> + endptr[1]);
> + exit(51);
> +descriptor:
> + fprintf(stderr, "ERROR: Unknown size descriptor '%c'\n", c);
> + exit(52);
> +invalid:
> + fprintf(stderr, "ERROR: Size value '%s' is invalid\n", s);
> + exit(53);
> +minus:
> + fprintf(stderr, "ERROR: Size value '%s' is less equal than 0\n", s);
> + exit(54);
Please get rid of the error codes, use exit(1). The messages should be
enough to find out what's wrong.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs-progs: Improve the parse_size() error message.
2014-05-28 17:20 ` David Sterba
@ 2014-05-29 0:41 ` Qu Wenruo
0 siblings, 0 replies; 5+ messages in thread
From: Qu Wenruo @ 2014-05-29 0:41 UTC (permalink / raw)
To: dsterba, linux-btrfs
Thanks for the commenting.
-------- Original Message --------
Subject: Re: [PATCH] btrfs-progs: Improve the parse_size() error message.
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2014年05月29日 01:20
> On Tue, May 20, 2014 at 03:51:45PM +0800, Qu Wenruo wrote:
>> When using parse_size(), even non-numeric value is passed, it will only
>> give error message "ERROR: size value is empty", which is quite
>> confusing for end users.
>>
>> This patch will introduce more meaningful error message for the
>> following new cases
>> 1) Invalid size string (non-numeric string)
>> 2) Minus size value (like "-1K")
> That's great. A few things below
>
>> u64 parse_size(char *s)
>> {
>> - int i;
>> char c;
>> u64 mult = 1;
>> -
>> - for (i = 0; s && s[i] && isdigit(s[i]); i++) ;
>> - if (!i) {
>> - fprintf(stderr, "ERROR: size value is empty\n");
>> - exit(50);
>> - }
>> -
>> - if (s[i]) {
>> - c = tolower(s[i]);
>> + long long int ret = 0;
>> + char *endptr;
>> +
>> + if (!s)
>> + goto empty;
>> + ret = strtoll(s, &endptr, 10);
>> - return strtoull(s, NULL, 10) * mult;
> Now it does not parse sizes from the range LLONG_MAX to ULLONG_MAX (8EiB
> to 16EiB). Though we don't have problems with such sizes in real life,
> I'd rather keep strtoull so it matches the advertised maximum filesystem
> size.
>
> Negative numbers can be simply identified by leading '-'. The only case
> where negative number is allowed is in 'fi resize', but the string is
> passed to kernel as-is.
In fact the leading '-' charactor judgment comes to me first, but "-0"
will make things complicated,
so I choose to use strtoll to deal with them.
To achieve the ULLONG_MAX, i would like to run strtoull after strtoll,
will it be OK for you?
>
>> +empty:
>> + fprintf(stderr, "ERROR: Size value is empty\n");
>> + exit(50);
>> +suffix:
>> + fprintf(stderr, "ERROR: Illegal suffix contains character '%c' in wrong position\n",
>> + endptr[1]);
>> + exit(51);
>> +descriptor:
>> + fprintf(stderr, "ERROR: Unknown size descriptor '%c'\n", c);
>> + exit(52);
>> +invalid:
>> + fprintf(stderr, "ERROR: Size value '%s' is invalid\n", s);
>> + exit(53);
>> +minus:
>> + fprintf(stderr, "ERROR: Size value '%s' is less equal than 0\n", s);
>> + exit(54);
> Please get rid of the error codes, use exit(1). The messages should be
> enough to find out what's wrong.
Exit(1) will make things quite easy. I will use them.
Thanks,
Qu
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs-progs: Improve the parse_size() error message.
2014-05-20 7:51 [PATCH] btrfs-progs: Improve the parse_size() error message Qu Wenruo
2014-05-28 17:20 ` David Sterba
@ 2014-05-28 21:07 ` Mike Fleetwood
2014-05-29 0:41 ` Qu Wenruo
1 sibling, 1 reply; 5+ messages in thread
From: Mike Fleetwood @ 2014-05-28 21:07 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
On 20 May 2014 08:51, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
> When using parse_size(), even non-numeric value is passed, it will only
> give error message "ERROR: size value is empty", which is quite
> confusing for end users.
>
> This patch will introduce more meaningful error message for the
> following new cases
> 1) Invalid size string (non-numeric string)
> 2) Minus size value (like "-1K")
>
> Also this patch will take full use of endptr returned by strtoll() to
> reduce unneeded loop.
>
> Reported-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
> utils.c | 53 +++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 33 insertions(+), 20 deletions(-)
>
> diff --git a/utils.c b/utils.c
> index 392c5cf..1cbe102 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1612,18 +1612,20 @@ scan_again:
>
> u64 parse_size(char *s)
> {
> - int i;
> char c;
> u64 mult = 1;
> -
> - for (i = 0; s && s[i] && isdigit(s[i]); i++) ;
> - if (!i) {
> - fprintf(stderr, "ERROR: size value is empty\n");
> - exit(50);
> - }
> -
> - if (s[i]) {
> - c = tolower(s[i]);
> + long long int ret = 0;
> + char *endptr;
> +
> + if (!s)
> + goto empty;
> + ret = strtoll(s, &endptr, 10);
> + if (endptr == s)
> + goto invalid;
> + if (endptr[0] && endptr[1])
> + goto suffix;
> + if (endptr[0]) {
> + c = tolower(endptr[0]);
> switch (c) {
> case 'e':
> mult *= 1024;
> @@ -1646,18 +1648,29 @@ u64 parse_size(char *s)
> case 'b':
> break;
> default:
> - fprintf(stderr, "ERROR: Unknown size descriptor "
> - "'%c'\n", c);
> - exit(1);
> + goto descriptor;
> }
> }
> - if (s[i] && s[i+1]) {
> - fprintf(stderr, "ERROR: Illegal suffix contains "
> - "character '%c' in wrong position\n",
> - s[i+1]);
> - exit(51);
> - }
> - return strtoull(s, NULL, 10) * mult;
> + ret *= mult;
> + if (ret <= 0)
> + goto minus;
> + return (u64) ret;
> +empty:
> + fprintf(stderr, "ERROR: Size value is empty\n");
> + exit(50);
> +suffix:
> + fprintf(stderr, "ERROR: Illegal suffix contains character '%c' in wrong position\n",
> + endptr[1]);
> + exit(51);
> +descriptor:
> + fprintf(stderr, "ERROR: Unknown size descriptor '%c'\n", c);
> + exit(52);
> +invalid:
> + fprintf(stderr, "ERROR: Size value '%s' is invalid\n", s);
> + exit(53);
> +minus:
> + fprintf(stderr, "ERROR: Size value '%s' is less equal than 0\n", s);
> + exit(54);
> }
>
> int open_file_or_dir3(const char *fname, DIR **dirstream, int open_flags)
> --
> 1.9.2
>
IMHO use of this "if condition goto print error exit" pattern makes
the code harder to read. Use of gotos is used when a function creates
state which needs tearing down in reverse on error before returning.
I think the errors should be printed at the point of detection. Like
this:
+ if (!s) {
+ fprintf(stderr, "ERROR: Size value is empty\n");
+ exit(1);
+ }
+ ret = strtoll(s, &endptr, 10);
+ if (endptr == s) {
+ fprintf(stderr, "ERROR: Size value '%s' is invalid\n", s);
+ exit(1);
+ }
etc.
Thanks,
Mike
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs-progs: Improve the parse_size() error message.
2014-05-28 21:07 ` Mike Fleetwood
@ 2014-05-29 0:41 ` Qu Wenruo
0 siblings, 0 replies; 5+ messages in thread
From: Qu Wenruo @ 2014-05-29 0:41 UTC (permalink / raw)
To: Mike Fleetwood; +Cc: linux-btrfs
-------- Original Message --------
Subject: Re: [PATCH] btrfs-progs: Improve the parse_size() error message.
From: Mike Fleetwood <mike.fleetwood@googlemail.com>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2014年05月29日 05:07
> On 20 May 2014 08:51, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>> When using parse_size(), even non-numeric value is passed, it will only
>> give error message "ERROR: size value is empty", which is quite
>> confusing for end users.
>>
>> This patch will introduce more meaningful error message for the
>> following new cases
>> 1) Invalid size string (non-numeric string)
>> 2) Minus size value (like "-1K")
>>
>> Also this patch will take full use of endptr returned by strtoll() to
>> reduce unneeded loop.
>>
>> Reported-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>> utils.c | 53 +++++++++++++++++++++++++++++++++--------------------
>> 1 file changed, 33 insertions(+), 20 deletions(-)
>>
>> diff --git a/utils.c b/utils.c
>> index 392c5cf..1cbe102 100644
>> --- a/utils.c
>> +++ b/utils.c
>> @@ -1612,18 +1612,20 @@ scan_again:
>>
>> u64 parse_size(char *s)
>> {
>> - int i;
>> char c;
>> u64 mult = 1;
>> -
>> - for (i = 0; s && s[i] && isdigit(s[i]); i++) ;
>> - if (!i) {
>> - fprintf(stderr, "ERROR: size value is empty\n");
>> - exit(50);
>> - }
>> -
>> - if (s[i]) {
>> - c = tolower(s[i]);
>> + long long int ret = 0;
>> + char *endptr;
>> +
>> + if (!s)
>> + goto empty;
>> + ret = strtoll(s, &endptr, 10);
>> + if (endptr == s)
>> + goto invalid;
>> + if (endptr[0] && endptr[1])
>> + goto suffix;
>> + if (endptr[0]) {
>> + c = tolower(endptr[0]);
>> switch (c) {
>> case 'e':
>> mult *= 1024;
>> @@ -1646,18 +1648,29 @@ u64 parse_size(char *s)
>> case 'b':
>> break;
>> default:
>> - fprintf(stderr, "ERROR: Unknown size descriptor "
>> - "'%c'\n", c);
>> - exit(1);
>> + goto descriptor;
>> }
>> }
>> - if (s[i] && s[i+1]) {
>> - fprintf(stderr, "ERROR: Illegal suffix contains "
>> - "character '%c' in wrong position\n",
>> - s[i+1]);
>> - exit(51);
>> - }
>> - return strtoull(s, NULL, 10) * mult;
>> + ret *= mult;
>> + if (ret <= 0)
>> + goto minus;
>> + return (u64) ret;
>> +empty:
>> + fprintf(stderr, "ERROR: Size value is empty\n");
>> + exit(50);
>> +suffix:
>> + fprintf(stderr, "ERROR: Illegal suffix contains character '%c' in wrong position\n",
>> + endptr[1]);
>> + exit(51);
>> +descriptor:
>> + fprintf(stderr, "ERROR: Unknown size descriptor '%c'\n", c);
>> + exit(52);
>> +invalid:
>> + fprintf(stderr, "ERROR: Size value '%s' is invalid\n", s);
>> + exit(53);
>> +minus:
>> + fprintf(stderr, "ERROR: Size value '%s' is less equal than 0\n", s);
>> + exit(54);
>> }
>>
>> int open_file_or_dir3(const char *fname, DIR **dirstream, int open_flags)
>> --
>> 1.9.2
>>
> IMHO use of this "if condition goto print error exit" pattern makes
> the code harder to read. Use of gotos is used when a function creates
> state which needs tearing down in reverse on error before returning.
> I think the errors should be printed at the point of detection. Like
> this:
>
> + if (!s) {
> + fprintf(stderr, "ERROR: Size value is empty\n");
> + exit(1);
> + }
> + ret = strtoll(s, &endptr, 10);
> + if (endptr == s) {
> + fprintf(stderr, "ERROR: Size value '%s' is invalid\n", s);
> + exit(1);
> + }
>
> etc.
>
> Thanks,
> Mike
Yes, that's true. I will change it to the original style.
Thanks,
Qu
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-05-29 0:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-20 7:51 [PATCH] btrfs-progs: Improve the parse_size() error message Qu Wenruo
2014-05-28 17:20 ` David Sterba
2014-05-29 0:41 ` Qu Wenruo
2014-05-28 21:07 ` Mike Fleetwood
2014-05-29 0:41 ` Qu Wenruo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).