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 <[email protected]> >>>> --- >>>> 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 [email protected] >>> 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
