| Message ID | 1392808674-21656-2-git-send-email-wangsl.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, 19 Feb 2014 19:17:51 +0800, Wang Shilong wrote: > There are many places that need parse string to u64 for btrfs commands, > in fact, we do such things *too casually*, using atoi/atol/atoll..is not > right at all, and even we don't check whether it is a valid string. > > Let's do everything more gracefully, we introduce a new helper > btrfs_strtoull() which will do all the necessary checks.If we fail to > parse string to u64, we will output message and exit directly, this is > something like what usage() is doing. It is ok to not return erro to > it's caller, because this function should be called when parsing arg > (just like usage!) > > Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> > --- > utils.c | 19 +++++++++++++++++++ > utils.h | 1 + > 2 files changed, 20 insertions(+) > > diff --git a/utils.c b/utils.c > index 97e23d5..0698d8d 100644 > --- a/utils.c > +++ b/utils.c > @@ -1520,6 +1520,25 @@ scan_again: > return 0; > } > > +u64 btrfs_strtoull(char *str, int base) > +{ > + u64 value; > + char *ptr_parse_end = NULL; > + char *ptr_str_end = str + strlen(str); > + > + value = strtoull(str, &ptr_parse_end, base); > + if (ptr_parse_end != ptr_str_end) { > + fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n", "not invalid" :) > + str); > + exit(1); > + } > + if (value == ULONG_MAX) { ULLONG_MAX or {errno = 0; value = strtoull(...); if (errno == ERANGE)...} > + fprintf(stderr, "ERROR: %s is out of range.\n", str); > + exit(1); > + } base = 0 is best BTW since it is able to read hex and octal numbers as well. I'd remove the base parameter to btrfs_strtoull() and always use 0. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Stefan, > On Wed, 19 Feb 2014 19:17:51 +0800, Wang Shilong wrote: >> There are many places that need parse string to u64 for btrfs commands, >> in fact, we do such things *too casually*, using atoi/atol/atoll..is not >> right at all, and even we don't check whether it is a valid string. >> >> Let's do everything more gracefully, we introduce a new helper >> btrfs_strtoull() which will do all the necessary checks.If we fail to >> parse string to u64, we will output message and exit directly, this is >> something like what usage() is doing. It is ok to not return erro to >> it's caller, because this function should be called when parsing arg >> (just like usage!) >> >> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> >> --- >> utils.c | 19 +++++++++++++++++++ >> utils.h | 1 + >> 2 files changed, 20 insertions(+) >> >> diff --git a/utils.c b/utils.c >> index 97e23d5..0698d8d 100644 >> --- a/utils.c >> +++ b/utils.c >> @@ -1520,6 +1520,25 @@ scan_again: >> return 0; >> } >> >> +u64 btrfs_strtoull(char *str, int base) >> +{ >> + u64 value; >> + char *ptr_parse_end = NULL; >> + char *ptr_str_end = str + strlen(str); >> + >> + value = strtoull(str, &ptr_parse_end, base); >> + if (ptr_parse_end != ptr_str_end) { >> + fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n", > > "not invalid" :) > oops >> + str); >> + exit(1); >> + } >> + if (value == ULONG_MAX) { > > ULLONG_MAX or {errno = 0; value = strtoull(...); if (errno == ERANGE)…} Yeah, i will use ULLONG_MAX instead ULONG to check^_^.. > > >> + fprintf(stderr, "ERROR: %s is out of range.\n", str); >> + exit(1); >> + } > > base = 0 is best BTW since it is able to read hex and octal numbers as > well. I'd remove the base parameter to btrfs_strtoull() and always use 0. Right, all comments addressed, thanks for your review.^_^ Thanks, Wang > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Wang, On 02/19/2014 12:17 PM, Wang Shilong wrote: > There are many places that need parse string to u64 for btrfs commands, > in fact, we do such things *too casually*, using atoi/atol/atoll..is not > right at all, and even we don't check whether it is a valid string. > > Let's do everything more gracefully, we introduce a new helper > btrfs_strtoull() which will do all the necessary checks.If we fail to > parse string to u64, we will output message and exit directly, this is > something like what usage() is doing. It is ok to not return erro to > it's caller, because this function should be called when parsing arg > (just like usage!) Please don't do that ! The error reporting of btrfs_strtoull is insufficient. In case of invalid value the user is not able to understand what is the the wrong parameter. This because the error reporting is handled by the function itself. We should improve the error messages, not hide them. From my point of view, you have only two choices: 1) change the api as u64 btrfs_strtoull(char *str, int *error) or int btrfs_strtoull(char *str, u64 *value) and let the function to return the error code in case of parsing error. The caller has the responsibility to inform the user of the error. 2) change the api as u64 btrfs_strtoull(char *str, char *parameter_name) so the function in case of error, is able to tell which parameters is wrong. I prefer the #1. BR G.Baroncelli > > Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> > --- > utils.c | 19 +++++++++++++++++++ > utils.h | 1 + > 2 files changed, 20 insertions(+) > > diff --git a/utils.c b/utils.c > index 97e23d5..0698d8d 100644 > --- a/utils.c > +++ b/utils.c > @@ -1520,6 +1520,25 @@ scan_again: > return 0; > } > > +u64 btrfs_strtoull(char *str, int base) > +{ > + u64 value; > + char *ptr_parse_end = NULL; > + char *ptr_str_end = str + strlen(str); > + > + value = strtoull(str, &ptr_parse_end, base); > + if (ptr_parse_end != ptr_str_end) { > + fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n", > + str); > + exit(1); > + } > + if (value == ULONG_MAX) { > + fprintf(stderr, "ERROR: %s is out of range.\n", str); > + exit(1); > + } > + return value; > +} > + > u64 parse_size(char *s) > { > int i; > diff --git a/utils.h b/utils.h > index 04b8c45..094f41d 100644 > --- a/utils.h > +++ b/utils.h > @@ -71,6 +71,7 @@ int pretty_size_snprintf(u64 size, char *str, size_t str_bytes); > int get_mountpt(char *dev, char *mntpt, size_t size); > int btrfs_scan_block_devices(int run_ioctl); > u64 parse_size(char *s); > +u64 btrfs_strtoull(char *str, int base); > int open_file_or_dir(const char *fname, DIR **dirstream); > void close_file_or_dir(int fd, DIR *dirstream); > int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args, >
Hi Goffredo, > Hi Wang, > > On 02/19/2014 12:17 PM, Wang Shilong wrote: >> There are many places that need parse string to u64 for btrfs commands, >> in fact, we do such things *too casually*, using atoi/atol/atoll..is not >> right at all, and even we don't check whether it is a valid string. >> >> Let's do everything more gracefully, we introduce a new helper >> btrfs_strtoull() which will do all the necessary checks.If we fail to >> parse string to u64, we will output message and exit directly, this is >> something like what usage() is doing. It is ok to not return erro to >> it's caller, because this function should be called when parsing arg >> (just like usage!) > > Please don't do that ! > > The error reporting of btrfs_strtoull is insufficient. > In case of invalid value the user is not able to > understand what is the the wrong parameter. This because the error > reporting is handled by the function itself. We should improve the error > messages, not hide them. > > > From my point of view, you have only two choices: > 1) change the api as > u64 btrfs_strtoull(char *str, int *error) > or > int btrfs_strtoull(char *str, u64 *value) > > and let the function to return the error code in case of parsing error. > The caller has the responsibility to inform the user of the error. > > 2) change the api as > > u64 btrfs_strtoull(char *str, char *parameter_name) > > so the function in case of error, is able to tell which parameters is wrong. > > I prefer the #1. I know what you are considering here, i was thinking the way you talked about. I chose this way for three reasons: #1 btrfs_strtoul() itself would output message why we fail before exit. #2 btrfs_strtoull() is called when arg parsing just like we use the function usage() which will call exit(1). #3 if we return error message to the caller, just think there are many caller that will deal the same thing, check and output error messages….which is a little polluted information…. So i think it is ok that we output message in btrfs_strtoull() itself and return directly.(It is ok because during arg parsing we don't involve memory allocation etc…) I understand your suggestions is more common, but for this case, I am more inclined to the current way to deal with the issue.^_^ Thanks, Wang > > BR > G.Baroncelli > >> >> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> >> --- >> utils.c | 19 +++++++++++++++++++ >> utils.h | 1 + >> 2 files changed, 20 insertions(+) >> >> diff --git a/utils.c b/utils.c >> index 97e23d5..0698d8d 100644 >> --- a/utils.c >> +++ b/utils.c >> @@ -1520,6 +1520,25 @@ scan_again: >> return 0; >> } >> >> +u64 btrfs_strtoull(char *str, int base) >> +{ >> + u64 value; >> + char *ptr_parse_end = NULL; >> + char *ptr_str_end = str + strlen(str); >> + >> + value = strtoull(str, &ptr_parse_end, base); >> + if (ptr_parse_end != ptr_str_end) { >> + fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n", >> + str); >> + exit(1); >> + } >> + if (value == ULONG_MAX) { >> + fprintf(stderr, "ERROR: %s is out of range.\n", str); >> + exit(1); >> + } >> + return value; >> +} >> + >> u64 parse_size(char *s) >> { >> int i; >> diff --git a/utils.h b/utils.h >> index 04b8c45..094f41d 100644 >> --- a/utils.h >> +++ b/utils.h >> @@ -71,6 +71,7 @@ int pretty_size_snprintf(u64 size, char *str, size_t str_bytes); >> int get_mountpt(char *dev, char *mntpt, size_t size); >> int btrfs_scan_block_devices(int run_ioctl); >> u64 parse_size(char *s); >> +u64 btrfs_strtoull(char *str, int base); >> int open_file_or_dir(const char *fname, DIR **dirstream); >> void close_file_or_dir(int fd, DIR *dirstream); >> int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args, >> > > > -- > gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it> > Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Wang, On 02/19/2014 05:08 PM, Wang Shilong wrote: > Hi Goffredo, > >> Hi Wang, >> >> On 02/19/2014 12:17 PM, Wang Shilong wrote: >>> There are many places that need parse string to u64 for btrfs commands, >>> in fact, we do such things *too casually*, using atoi/atol/atoll..is not >>> right at all, and even we don't check whether it is a valid string. >>> >>> Let's do everything more gracefully, we introduce a new helper >>> btrfs_strtoull() which will do all the necessary checks.If we fail to >>> parse string to u64, we will output message and exit directly, this is >>> something like what usage() is doing. It is ok to not return erro to >>> it's caller, because this function should be called when parsing arg >>> (just like usage!) >> >> Please don't do that ! >> >> The error reporting of btrfs_strtoull is insufficient. >> In case of invalid value the user is not able to >> understand what is the the wrong parameter. This because the error >> reporting is handled by the function itself. We should improve the error >> messages, not hide them. >> >> >> From my point of view, you have only two choices: >> 1) change the api as >> u64 btrfs_strtoull(char *str, int *error) >> or >> int btrfs_strtoull(char *str, u64 *value) >> >> and let the function to return the error code in case of parsing error. >> The caller has the responsibility to inform the user of the error. >> >> 2) change the api as >> >> u64 btrfs_strtoull(char *str, char *parameter_name) >> >> so the function in case of error, is able to tell which parameters is wrong. >> >> I prefer the #1. > > I know what you are considering here, i was thinking the way you talked about. > I chose this way for three reasons: > > #1 btrfs_strtoul() itself would output message why we fail before > exit. The error message says that the value is out of range. But doesn't tell which is the parameter involved. If you have a command which has several arguments, and the user pass a string instead of a number, the error returned doesn't tell which argument is wrong. This is the reason of my complaint. At least add a fourth parameter which contains the name of the parameter parsed in order to improve the error message. I.E. subvol_id = btrfs_strtoull(argv[i], 10, "subvolume ID"); If the user pass a path instead of a number the error message would be ERROR: xxxx is not a valid unsigned long long integer for the parameter 'subvolume ID'. Or something like that. > #2 btrfs_strtoull() is called when arg parsing just like we use > the function usage() which will call exit(1). Yes this could be a reasonable tread off, even I would prefer a more explicit name of the function (like argv_strtoull) in order to highlight that it is a special function which could exit. > #3 if we return error > message to the caller, just think there are many caller that will > deal the same thing, check and output error messages….which is a > little polluted information…. > > So i think it is ok that we output message in btrfs_strtoull() itself > and return directly.(It is ok because during arg parsing we don't > involve memory allocation etc…) > > I understand your suggestions is more common, but for this case, I > am more inclined to the current way to deal with the issue.^_^> > Thanks, > Wang >> >> BR >> G.Baroncelli >> >>> >>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> >>> --- >>> utils.c | 19 +++++++++++++++++++ >>> utils.h | 1 + >>> 2 files changed, 20 insertions(+) >>> >>> diff --git a/utils.c b/utils.c >>> index 97e23d5..0698d8d 100644 >>> --- a/utils.c >>> +++ b/utils.c >>> @@ -1520,6 +1520,25 @@ scan_again: >>> return 0; >>> } >>> >>> +u64 btrfs_strtoull(char *str, int base) >>> +{ >>> + u64 value; >>> + char *ptr_parse_end = NULL; >>> + char *ptr_str_end = str + strlen(str); >>> + >>> + value = strtoull(str, &ptr_parse_end, base); >>> + if (ptr_parse_end != ptr_str_end) { >>> + fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n", >>> + str); >>> + exit(1); >>> + } >>> + if (value == ULONG_MAX) { >>> + fprintf(stderr, "ERROR: %s is out of range.\n", str); >>> + exit(1); >>> + } >>> + return value; >>> +} >>> + >>> u64 parse_size(char *s) >>> { >>> int i; >>> diff --git a/utils.h b/utils.h >>> index 04b8c45..094f41d 100644 >>> --- a/utils.h >>> +++ b/utils.h >>> @@ -71,6 +71,7 @@ int pretty_size_snprintf(u64 size, char *str, size_t str_bytes); >>> int get_mountpt(char *dev, char *mntpt, size_t size); >>> int btrfs_scan_block_devices(int run_ioctl); >>> u64 parse_size(char *s); >>> +u64 btrfs_strtoull(char *str, int base); >>> int open_file_or_dir(const char *fname, DIR **dirstream); >>> void close_file_or_dir(int fd, DIR *dirstream); >>> int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args, >>> >> >> >> -- >> gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it> >> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >
Hi Goffredo, > Hi Wang, > On 02/19/2014 05:08 PM, Wang Shilong wrote: >> Hi Goffredo, >> >>> Hi Wang, >>> >>> On 02/19/2014 12:17 PM, Wang Shilong wrote: >>>> There are many places that need parse string to u64 for btrfs commands, >>>> in fact, we do such things *too casually*, using atoi/atol/atoll..is not >>>> right at all, and even we don't check whether it is a valid string. >>>> >>>> Let's do everything more gracefully, we introduce a new helper >>>> btrfs_strtoull() which will do all the necessary checks.If we fail to >>>> parse string to u64, we will output message and exit directly, this is >>>> something like what usage() is doing. It is ok to not return erro to >>>> it's caller, because this function should be called when parsing arg >>>> (just like usage!) >>> >>> Please don't do that ! >>> >>> The error reporting of btrfs_strtoull is insufficient. >>> In case of invalid value the user is not able to >>> understand what is the the wrong parameter. This because the error >>> reporting is handled by the function itself. We should improve the error >>> messages, not hide them. >>> >>> >>> From my point of view, you have only two choices: >>> 1) change the api as >>> u64 btrfs_strtoull(char *str, int *error) >>> or >>> int btrfs_strtoull(char *str, u64 *value) >>> >>> and let the function to return the error code in case of parsing error. >>> The caller has the responsibility to inform the user of the error. >>> >>> 2) change the api as >>> >>> u64 btrfs_strtoull(char *str, char *parameter_name) >>> >>> so the function in case of error, is able to tell which parameters is wrong. >>> >>> I prefer the #1. >> >> I know what you are considering here, i was thinking the way you talked about. >> I chose this way for three reasons: >> >> #1 btrfs_strtoul() itself would output message why we fail before >> exit. > The error message says that the value is out of range. But doesn't tell which > is the parameter involved. > If you have a command which has several arguments, and the user pass a string > instead of a number, the error returned doesn't tell which argument is wrong. > This is the reason of my complaint. > > At least add a fourth parameter which contains the name of the parameter > parsed in order to improve the error message. > > I.E. > > subvol_id = btrfs_strtoull(argv[i], 10, "subvolume ID"); > > If the user pass a path instead of a number the error message would be > > ERROR: xxxx is not a valid unsigned long long integer for the parameter 'subvolume ID'. This is more friendly.^_^ > > Or something like that. > > > >> #2 btrfs_strtoull() is called when arg parsing just like we use >> the function usage() which will call exit(1). > Yes this could be a reasonable tread off, even I would prefer a more > explicit name of the function (like argv_strtoull) in order to highlight > that it is a special function which could exit. Fair enough! So we reach an agreement, new helper will be something like the following. u64 argv_strtoull(const char *arg, const char * argv_name); Regards, Wang > >> #3 if we return error >> message to the caller, just think there are many caller that will >> deal the same thing, check and output error messages….which is a >> little polluted information…. > >> >> So i think it is ok that we output message in btrfs_strtoull() itself >> and return directly.(It is ok because during arg parsing we don't >> involve memory allocation etc…) >> >> I understand your suggestions is more common, but for this case, I >> am more inclined to the current way to deal with the issue.^_^> >> Thanks, >> Wang >>> >>> BR >>> G.Baroncelli >>> >>>> >>>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> >>>> --- >>>> utils.c | 19 +++++++++++++++++++ >>>> utils.h | 1 + >>>> 2 files changed, 20 insertions(+) >>>> >>>> diff --git a/utils.c b/utils.c >>>> index 97e23d5..0698d8d 100644 >>>> --- a/utils.c >>>> +++ b/utils.c >>>> @@ -1520,6 +1520,25 @@ scan_again: >>>> return 0; >>>> } >>>> >>>> +u64 btrfs_strtoull(char *str, int base) >>>> +{ >>>> + u64 value; >>>> + char *ptr_parse_end = NULL; >>>> + char *ptr_str_end = str + strlen(str); >>>> + >>>> + value = strtoull(str, &ptr_parse_end, base); >>>> + if (ptr_parse_end != ptr_str_end) { >>>> + fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n", >>>> + str); >>>> + exit(1); >>>> + } >>>> + if (value == ULONG_MAX) { >>>> + fprintf(stderr, "ERROR: %s is out of range.\n", str); >>>> + exit(1); >>>> + } >>>> + return value; >>>> +} >>>> + >>>> u64 parse_size(char *s) >>>> { >>>> int i; >>>> diff --git a/utils.h b/utils.h >>>> index 04b8c45..094f41d 100644 >>>> --- a/utils.h >>>> +++ b/utils.h >>>> @@ -71,6 +71,7 @@ int pretty_size_snprintf(u64 size, char *str, size_t str_bytes); >>>> int get_mountpt(char *dev, char *mntpt, size_t size); >>>> int btrfs_scan_block_devices(int run_ioctl); >>>> u64 parse_size(char *s); >>>> +u64 btrfs_strtoull(char *str, int base); >>>> int open_file_or_dir(const char *fname, DIR **dirstream); >>>> void close_file_or_dir(int fd, DIR *dirstream); >>>> int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args, >>>> >>> >>> >>> -- >>> gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it> >>> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> > > > -- > gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it> > Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2/19/14, 10:31 AM, Goffredo Baroncelli wrote: ... > The error message says that the value is out of range. But doesn't tell which > is the parameter involved. > If you have a command which has several arguments, and the user pass a string > instead of a number, the error returned doesn't tell which argument is wrong. > This is the reason of my complaint. > > At least add a fourth parameter which contains the name of the parameter > parsed in order to improve the error message. > > I.E. > > subvol_id = btrfs_strtoull(argv[i], 10, "subvolume ID"); > > If the user pass a path instead of a number the error message would be > > ERROR: xxxx is not a valid unsigned long long integer for the parameter 'subvolume ID'. > > Or something like that. I'm not so sure that this is needed. Adding it makes the code a little more complicated, and requires each caller to send in a string which may get out of sync with the actual argument name (or the manpage, or the help/usage text). The user has *just* typed it in, so it won't be too hard to see which one was wrong even without naming the parameter, i.e. - # btrfs foo-command 12345 0 123notanumber Error: 123notanumber is not a valid numeric value or # btrfs baz-command 0xFFFFFFFFFFFFFFFF 12345 0 Error: numeric value 0xFFFFFFFFFFFFFFFF is too large. would probably suffice, without bothering to name the parameter. I'd also suggest not referring to "unsigned long long" or "out of range" which might not mean much to people who aren't programmers; "not a valid value" or "too large" might be clearer. Also, what if someone enters a negative number? Right now it happily accepts it and converts it to an unsigned value: # ./test-64bit -123 Parsed as 18446744073709551493 but an inadvertent negative sign might lead to highly unexpected results. Just my $0.02. -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/20/2014 01:23 AM, Eric Sandeen wrote: > On 2/19/14, 10:31 AM, Goffredo Baroncelli wrote: > ... > > >> The error message says that the value is out of range. But doesn't tell which >> is the parameter involved. >> If you have a command which has several arguments, and the user pass a string >> instead of a number, the error returned doesn't tell which argument is wrong. >> This is the reason of my complaint. >> >> At least add a fourth parameter which contains the name of the parameter >> parsed in order to improve the error message. >> >> I.E. >> >> subvol_id = btrfs_strtoull(argv[i], 10, "subvolume ID"); >> >> If the user pass a path instead of a number the error message would be >> >> ERROR: xxxx is not a valid unsigned long long integer for the parameter 'subvolume ID'. >> >> Or something like that. > I'm not so sure that this is needed. Adding it makes the code a little more complicated, > and requires each caller to send in a string which may get out of sync with the actual > argument name (or the manpage, or the help/usage text). > > The user has *just* typed it in, so it won't be too hard to see which one > was wrong even without naming the parameter, i.e. - > > # btrfs foo-command 12345 0 123notanumber > Error: 123notanumber is not a valid numeric value > > or > > # btrfs baz-command 0xFFFFFFFFFFFFFFFF 12345 0 > Error: numeric value 0xFFFFFFFFFFFFFFFF is too large. > > would probably suffice, without bothering to name the parameter. Sounds more reasonable.:-) > > I'd also suggest not referring to "unsigned long long" or "out of range" which > might not mean much to people who aren't programmers; "not a valid value" > or "too large" might be clearer. Will update it. > > Also, what if someone enters a negative number? Right now it happily > accepts it and converts it to an unsigned value: > > # ./test-64bit -123 > Parsed as 18446744073709551493 So Let's add a '-' check before calling strtoull. something like: if (str[0] == '-') fprintf(stderr, "ERROR: this may be a negative integer: %s\n", str); Thanks, Wang > > but an inadvertent negative sign might lead to highly unexpected results. > > Just my $0.02. > > -Eric > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 19, 2014 at 07:17:51PM +0800, Wang Shilong wrote: > +u64 btrfs_strtoull(char *str, int base) > +{ > + u64 value; > + char *ptr_parse_end = NULL; > + char *ptr_str_end = str + strlen(str); > + > + value = strtoull(str, &ptr_parse_end, base); > + if (ptr_parse_end != ptr_str_end) { > + fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n", > + str); > + exit(1); Calling exit() in a helper function makes is unsuitable as a generic helper or for use from any library function, though all the places where it's used are in main() so it's similar to usage(), and makes handling errors in command line arguments easier. I'm still concerned about the generic function name, but don't have an idea how to fix it atm. > + } > + if (value == ULONG_MAX) { > + fprintf(stderr, "ERROR: %s is out of range.\n", str); > + exit(1); > + } > + return value; > +} -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/utils.c b/utils.c index 97e23d5..0698d8d 100644 --- a/utils.c +++ b/utils.c @@ -1520,6 +1520,25 @@ scan_again: return 0; } +u64 btrfs_strtoull(char *str, int base) +{ + u64 value; + char *ptr_parse_end = NULL; + char *ptr_str_end = str + strlen(str); + + value = strtoull(str, &ptr_parse_end, base); + if (ptr_parse_end != ptr_str_end) { + fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n", + str); + exit(1); + } + if (value == ULONG_MAX) { + fprintf(stderr, "ERROR: %s is out of range.\n", str); + exit(1); + } + return value; +} + u64 parse_size(char *s) { int i; diff --git a/utils.h b/utils.h index 04b8c45..094f41d 100644 --- a/utils.h +++ b/utils.h @@ -71,6 +71,7 @@ int pretty_size_snprintf(u64 size, char *str, size_t str_bytes); int get_mountpt(char *dev, char *mntpt, size_t size); int btrfs_scan_block_devices(int run_ioctl); u64 parse_size(char *s); +u64 btrfs_strtoull(char *str, int base); int open_file_or_dir(const char *fname, DIR **dirstream); void close_file_or_dir(int fd, DIR *dirstream); int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args,
There are many places that need parse string to u64 for btrfs commands, in fact, we do such things *too casually*, using atoi/atol/atoll..is not right at all, and even we don't check whether it is a valid string. Let's do everything more gracefully, we introduce a new helper btrfs_strtoull() which will do all the necessary checks.If we fail to parse string to u64, we will output message and exit directly, this is something like what usage() is doing. It is ok to not return erro to it's caller, because this function should be called when parsing arg (just like usage!) Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> --- utils.c | 19 +++++++++++++++++++ utils.h | 1 + 2 files changed, 20 insertions(+)