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

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 4777 bytes --]

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.  */

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).

> > +
> > +#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: 488 bytes --]