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.f...@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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
