[PATCH v6 2/5] linux: Use long time_t __getitimer/__setitimer

[Adhemerval Zanella]

On 30/03/2020 13:17, Alistair Francis wrote:
> On Mon, Mar 30, 2020 at 7:49 AM Adhemerval Zanella via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
>>
>>
>>
>> On 29/03/2020 19:19, Stepan Golosunov wrote:
>>> 29.03.2020 в 11:17:17 -0700 Alistair Francis написал:
>>>> On Sun, Mar 29, 2020 at 2:59 AM Stepan Golosunov <stepan@golosunov.pp.ru> wrote:
>>>>>
>>>>> 28.03.2020 в 08:22:46 -0700 Alistair Francis написал:
>>>>>> --- /dev/null
>>>>>> +++ b/sysdeps/unix/sysv/linux/getitimer.c
>>>>>
>>>>>> +int
>>>>>> +__getitimer64 (__itimer_which_t which, struct __itimerval64 *curr_value)
>>>>>> +{
>>>>>> +#if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64
>>>>>> +  return INLINE_SYSCALL_CALL (getitimer, which, curr_value);
>>>>>> +#else
>>>>>> +  struct __itimerval32 curr_value_32;
>>>>>> +
>>>>>> +  if (INLINE_SYSCALL_CALL (getitimer, which, &curr_value_32) == -1)
>>>>>> +    return -1;
>>>>>> +
>>>>>> +  curr_value->it_interval
>>>>>> +    = valid_timeval32_to_timeval64 (curr_value_32.it_interval);
>>>>>> +  curr_value->it_value
>>>>>> +    = valid_timeval32_to_timeval64 (curr_value_32.it_value);
>>>>>> +  return 0;
>>>>>> +#endif
>>>>>> +}
>>>>>> +
>>>>>> +#if __TIMESIZE != 64
>>>>>> +libc_hidden_def (__getitimer64)
>>>>>> +int
>>>>>> +__getitimer (__itimer_which_t which, struct itimerval *curr_value)
>>>>>> +{
>>>>>> +  struct __itimerval64 val64;
>>>>>> +
>>>>>> +  val64.it_interval
>>>>>> +    = valid_timeval_to_timeval64 (curr_value->it_interval);
>>>>>> +  val64.it_value
>>>>>> +    = valid_timeval_to_timeval64 (curr_value->it_value);
>>>>>> +
>>>>>> +  return __getitimer64 (which, &val64);
>>>>>> +}
>>>>>> +#endif
>>>>>> +weak_alias (__getitimer, getitimer)
>>>>>
>>>>> __getitimer treats curr_value as input-only variable, while it's an
>>>>> output-only one in __getitimer64.  This won't work.
>>>>
>>>> I'm not sure what you mean here, can you please elaborate?
>>>
>>> __getitimer (…, &curr_value) will never write to curr_value (or do
>>> anything useful with it at all); while
>>> __getitimer64 (…, &curr_value) will write to curr_value as expected.
>>>
>>> Conversion in __getitimer shold be in opposite direction and after
>>> __getitimer64 call.
>>>
>>
>> Indeed, it should be:
>>
>>   int
>>   __getitimer64 (__itimer_which_t which, struct __itimerval64 *curr_value)
>>   {
>>     [...]
>>   }
>>
>>   #if __TIMESIZE != 64
>>   libc_hidden_def (__getitimer64)
>>
>>   int
>>   __getitimer (__itimer_which_t which, struct itimerval *curr_value)
>>   {
>>     struct __itimerval64 val64;
>>     if (__getitimer64 (which, &val64) != 0)
>>       return -1;
>>
>>     curr_value->it_interval
>>       = valid_timeval64_to_timeval (val64->it_interval);
>>     curr_value->it_value
>>       = valid_timeval64_to_timeval (val64->it_value);
>>
>>     return 0;
>>   }
>>   #endif
> 
> I changed it to this (which is very similar to above). Good catch on that.
> 
> #if __TIMESIZE != 64
> libc_hidden_def (__getitimer64)
> int
> __getitimer (__itimer_which_t which, struct itimerval *curr_value)
> {
>   struct __itimerval64 val64;
>   int ret = __getitimer64 (which, &val64);

There is no need to actually check the return value, POSIX states it
return -1 on failure (and __getitimer64 will set errno accordingly).

> 
>   if (ret == 0 && curr_value)

Again there is no need to check if 'curr_value', neither POSIX or
kernels add such constraints (and kernel does return EFAULT in
such case).

>     {
>       curr_value->it_interval
>         = valid_timeval64_to_timeval (val64.it_interval);
>       curr_value->it_value
>         = valid_timeval64_to_timeval (val64.it_value);
>     }
> 
>   return ret;