[CRIU] [PATCH v2 1/2] util: xatol() and xatoi() helpers introduced

[Stanislav Kinsburskiy]

26.09.2017 22:30, Andrei Vagin пишет:
> On Tue, Sep 26, 2017 at 11:16:14AM +0200, Stanislav Kinsburskiy wrote:
>>
>>
>> 25.09.2017 20:19, Andrei Vagin пишет:
>>> On Mon, Sep 25, 2017 at 11:07:11AM +0200, Stanislav Kinsburskiy wrote:
>>>>
>>>>
>>>> 23.09.2017 00:30, Andrei Vagin пишет:
>>>>> On Wed, Sep 13, 2017 at 04:48:50PM +0300, Stanislav Kinsburskiy wrote:
>>>>>> These helpers are safe versions of atol() and atoi() respectively.
>>>>>> And they check for overflow and NAN errors
>>>>>>
>>>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky at virtuozzo.com>
>>>>>> ---
>>>>>>  criu/include/util.h |    3 +++
>>>>>>  criu/util.c         |   38 ++++++++++++++++++++++++++++++++++++++
>>>>>>  2 files changed, 41 insertions(+)
>>>>>>
>>>>>> diff --git a/criu/include/util.h b/criu/include/util.h
>>>>>> index 1fc20db..20684cc 100644
>>>>>> --- a/criu/include/util.h
>>>>>> +++ b/criu/include/util.h
>>>>>> @@ -280,6 +280,9 @@ void tcp_cork(int sk, bool on);
>>>>>>  
>>>>>>  const char *ns_to_string(unsigned int ns);
>>>>>>  
>>>>>> +int xatol(const char *string, long *number);
>>>>>> +int xatoi(const char *string, int *number);
>>>>>> +
>>>>>>  char *xstrcat(char *str, const char *fmt, ...)
>>>>>>  	__attribute__ ((__format__ (__printf__, 2, 3)));
>>>>>>  char *xsprintf(const char *fmt, ...)
>>>>>> diff --git a/criu/util.c b/criu/util.c
>>>>>> index b5a161f..7d60617 100644
>>>>>> --- a/criu/util.c
>>>>>> +++ b/criu/util.c
>>>>>> @@ -58,6 +58,44 @@
>>>>>>  
>>>>>>  #define VMA_OPT_LEN	128
>>>>>>  
>>>>>> +static int xatol_base(const char *string, long *number, int base)
>>>>>> +{
>>>>>> +	char *endptr;
>>>>>> +	long nr;
>>>>>> +
>>>>>> +	errno = 0;
>>>>>> +	nr = strtol(string, &endptr, base);
>>>>>> +	if ((errno == ERANGE && (nr == LONG_MAX || nr == LONG_MIN))
>>>>>> +			|| (errno != 0 && nr == 0)) {
>>>>>> +		pr_perror("failed to convert string");
> 
> pr_perror("failed to convert string: %s", string);
> 

Ok, now got it.

>>>>>
>>>>> It is better to print this string in a error message
>>>>>
>>>>
>>>>
>>>> Well,on the other hand how we can distinguish between this error case above and EINVAL, returned below, when string is not a number?
>>>
>>> I am not sure that I understand what you mean. Error messages are
>>> different, is it not enough?
>>>
>>
>> They are not. Function strtol can return EINVAL.
>> And EINVAL is also returned below on endptr check.
> 
> I sugested to print a string in a error message, why you started talking
> about error codes? Why don't you return ERANGE here?
> 

It was misunderstanding. Will add the string to the error message.

>>
>>>>
>>>>
>>>>>> +		return -EINVAL;
>>>>>> +	}
>>>>>> +
>>>>>> +	if ((endptr == string) || (*endptr != '\0')) {
>>>>>> +		pr_err("String is not a number: '%s'\n", string);
>>>>>> +		return -EINVAL;
>>
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^ here.
>>
>> But here it can be converted to some other error though... ENOTSUP for instance, if you insist on moving the print outside the helper.
>>
>>
>>>>>> +	}
>>>>>> +	*number = nr;
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +int xatol(const char *string, long *number)
>>>>>> +{
>>>>>> +	return xatol_base(string, number, 10);
>>>>>> +}
>>>>>> +
>>>>>> +
>>>>>> +int xatoi(const char *string, int *number)
>>>>>> +{
>>>>>> +	long tmp;
>>>>>> +	int err;
>>>>>> +
>>>>>> +	err = xatol(string, &tmp);
>>>>>
>>>>> Should we check that tmp is in [INT_MIN, INT_MAX]?
>>>>
>>>>
>>>> Well, looks like we shouldn't. Atoi will return interger even if string contains long.
>>>
>>> So why do we need to check that a value is in [LONG_MIN, LONG_MAX] for
>>> xatol? Why do we need these hellpers?
>>>
>>
>> 1) In case of xatol we check the string can be converted in arch-size variable.
>> 2) In case of xatoi it's not needed, because number can be converted to int by simply zeroing high 32 bits. You don't check number for int, when casting like this "(int)long_number", don't you?
> 
> It is not convince me. xatoi and xtol have to be symetrical, if one of
> them check a range, another one should check a range too (IMHO)
> 

Well, I'll add the check, if you wish.

>> 3) These helpers do convert strings to numbers *properly* and taking errors into account.
>> I understand, that CRIU relies on the fact that kernel is much less error-prone and if we know, that is has to return int (long) we can rely on it. But I don't think that it's a safe approach. Do you?
> 
> I am agree and this point contradicts with your explanation.
> 
> If the kernel has to return int, but it returns a value bigger than
> INT_MAX, xatoi has to return an error, doesn't it?
> 

Well, let's do it this way.