Re: [PATCH v2 6/7] y2038: linux: Provide __ntp_gettime64 implementation

[Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org>]

[-- Attachment #1.1: Type: text/plain, Size: 5094 bytes --]



On 19/05/2020 17:20, Lukasz Majewski wrote:
> Hi Adhemerval,
> 
>> On 08/05/2020 11:56, Lukasz Majewski wrote:
>>> This patch provides new __ntp_gettime64 explicit 64 bit function
>>> for getting time parameters via NTP interface.
>>>
>>> Internally, the __clock_adjtime64 syscall is used instead of
>>> __adjtimex. This patch is necessary for having architectures with
>>> __WORDSIZE == 32 Y2038 safe.
>>>
>>> Moreover, a 32 bit version - __ntp_gettime has been refactored to
>>> internally use __ntp_gettime64.
>>>
>>> The __ntp_gettime is now supposed to be used on systems still
>>> supporting 32 bit time (__TIMESIZE != 64) - hence the necessary
>>> conversions between struct ntptimeval and 64 bit struct
>>> __ntptimeval64.
>>>
>>> Build tests:
>>> ./src/scripts/build-many-glibcs.py glibcs
>>>
>>> Run-time tests:
>>> - Run specific tests on ARM/x86 32bit systems (qemu):
>>>   https://github.com/lmajewski/meta-y2038 and run tests:
>>>   https://github.com/lmajewski/y2038-tests/commits/master
>>>
>>> Above tests were performed with Y2038 redirection applied as well
>>> as without to test the proper usage of both __ntp_gettime64 and
>>> __ntp_gettime.  
>>
>> Ok with a doubt below.
>>
>> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>>
>>> ---
>>>  sysdeps/unix/sysv/linux/include/sys/timex.h |  4 ++++
>>>  sysdeps/unix/sysv/linux/ntp_gettime.c       | 24
>>> ++++++++++++++++++--- 2 files changed, 25 insertions(+), 3
>>> deletions(-)
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/include/sys/timex.h
>>> b/sysdeps/unix/sysv/linux/include/sys/timex.h index
>>> e762e03230..ef53515803 100644 ---
>>> a/sysdeps/unix/sysv/linux/include/sys/timex.h +++
>>> b/sysdeps/unix/sysv/linux/include/sys/timex.h @@ -33,6 +33,7 @@
>>> libc_hidden_proto (__adjtimex) #   define __clock_adjtime64
>>> __clock_adjtime #   define ___adjtimex64 ___adjtimex
>>>  #   define __ntptimeval64 ntptimeval
>>> +#   define __ntp_gettime64 __ntp_gettime
>>>  #  else
>>>  
>>>  struct __timex64
>>> @@ -91,6 +92,9 @@ struct __ntptimeval64
>>>    long int __glibc_reserved3;
>>>    long int __glibc_reserved4;
>>>  };
>>> +extern int __ntp_gettime64 (struct __ntptimeval64 *ntv);
>>> +libc_hidden_proto (__ntp_gettime64)
>>> +
>>>  #  endif
>>>  
>>>  /* Convert a known valid struct timex into a struct __timex64.  */
>>>  
>>
>> Ok.
>>
>>> diff --git a/sysdeps/unix/sysv/linux/ntp_gettime.c
>>> b/sysdeps/unix/sysv/linux/ntp_gettime.c index
>>> c8d6a197dc..21aeffadeb 100644 ---
>>> a/sysdeps/unix/sysv/linux/ntp_gettime.c +++
>>> b/sysdeps/unix/sysv/linux/ntp_gettime.c @@ -17,6 +17,7 @@
>>>  
>>>  #define ntp_gettime ntp_gettime_redirect
>>>  
>>> +#include <time.h>
>>>  #include <sys/timex.h>
>>>  
>>>  #undef ntp_gettime
>>> @@ -27,15 +28,32 @@
>>>  
>>>  
>>>  int
>>> -ntp_gettime (struct ntptimeval *ntv)
>>> +__ntp_gettime64 (struct __ntptimeval64 *ntv)
>>>  {
>>> -  struct timex tntx;
>>> +  struct __timex64 tntx;
>>>    int result;
>>>  
>>>    tntx.modes = 0;
>>> -  result = __adjtimex (&tntx);
>>> +  result = __clock_adjtime64 (CLOCK_REALTIME, &tntx);
>>>    ntv->time = tntx.time;
>>>    ntv->maxerror = tntx.maxerror;
>>>    ntv->esterror = tntx.esterror;
>>>    return result;
>>>  }  
>>
>> Ok. Maybe add a comment stating that using CLOCK_REALTIME should
>> not make the function fail with EINVAL, ENODEV, or EOPNOTSUPP.
>> I am not sure about EPERM in this situation, should we check for
>> that and avoid seeting NTV in such situation?
>>
> 
> I will add following comment:
> 
> /* Using the CLOCK_REALTIME with __clock_adjtime64 (as a replacement
> for Y2038 unsafe adjtimex) will not make the function fail with EINVAL,
> ENODEV, or EOPNOTSUPP.  */

Maybe:

  /* clock_adjtime64 with CLOCK_REALTIME does not trigger EINVAL,
     ENODEV, or EOPNOTSUPP.  It might still trigger EPERM.  */

> 
> Regarding the EPERM:
> 
> The adjtimex also could return EPERM:
> http://man7.org/linux/man-pages/man2/adjtimex.2.html
> 
> which would be propagated to caller of ntp_gettime. In this case the
> ntv structure would get updated.
> 
> If we want to preserve the same behavior, it would be correct to leave
> the code as is (and ntv would get updated anyway).

Alight, we can track this in another patch.

> 
>>> +
>>> +#if __TIMESIZE != 64
>>> +libc_hidden_def (__ntp_gettime64)
>>> +
>>> +int
>>> +__ntp_gettime (struct ntptimeval *ntv)
>>> +{
>>> +  struct __ntptimeval64 ntv64;
>>> +  int result;
>>> +
>>> +  result = __ntp_gettime64 (&ntv64);
>>> +  *ntv = valid_ntptimeval64_to_ntptimeval (ntv64);
>>> +
>>> +  return result;
>>> +}
>>> +#endif
>>> +strong_alias (__ntp_gettime, ntp_gettime)
>>>   
>>
>> Ok.
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]