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

[Wang Shilong <wangsl.fnst@cn.fujitsu.com>]

On 02/20/2014 01:23 AM, Eric Sandeen wrote:
> On 2/19/14, 10:31 AM, Goffredo Baroncelli wrote:
> ...
>
>   
>> 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.
> I'm not so sure that this is needed.  Adding it makes the code a little more complicated,
> and requires each caller to send in a string which may get out of sync with the actual
> argument name (or the manpage, or the help/usage text).
>
> The user has *just* typed it in, so it won't be too hard to see which one
> was wrong even without naming the parameter, i.e. -
>
> # btrfs foo-command 12345 0 123notanumber
> Error: 123notanumber is not a valid numeric value
>
> or
>
> # btrfs baz-command 0xFFFFFFFFFFFFFFFF 12345 0
> Error: numeric value 0xFFFFFFFFFFFFFFFF is too large.
>
> would probably suffice, without bothering to name the parameter.
Sounds more reasonable.:-)
>
> I'd also suggest not referring to "unsigned long long" or "out of range" which
> might not mean much to people who aren't programmers; "not a valid value"
> or "too large" might be clearer.
Will update it.
>
> Also, what if someone enters a negative number?  Right now it happily
> accepts it and converts it to an unsigned value:
>
> # ./test-64bit -123
> Parsed as 18446744073709551493
So Let's add a '-' check before calling strtoull. something like:

if (str[0] == '-')
     fprintf(stderr, "ERROR: this may be a negative integer: %s\n", str);

Thanks,
Wang
>
> but an inadvertent negative sign might lead to highly unexpected results.
>
> Just my $0.02.
>
> -Eric
> --
> 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
>