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

[Goffredo Baroncelli <kreijack@inwind.it>]

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


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