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