Re: [PATCH 1/4] Btrfs-progs: new helper to parse string to u64 for btrfs

[Goffredo Baroncelli <kreijack@libero.it>]

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,
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5