Re: [RFC v4 06/24] sysdeps/timespec_get: Use clock_gettime64 if avaliable

[Alistair Francis <alistair23 at gmail dot com>]

On Thu, Aug 15, 2019 at 12:46 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Thu, 15 Aug 2019, Alistair Francis wrote:
>
> > Is this the type of layout you are looking for? (untested, just a general idea)
>
> I'm not convinced making timespec_get into a wrapper round another
> function is a good thing.
>
> You need to end up, in the __TIMESIZE == 32 case, with two functions that
> have the public timespec_get semantics - not two internal functions and a
> single wrapper - in preparation for when we support _TIME_BITS == 64.  So
> I'd expect defining __timespec_get64 as a function with the public
> semantics (not as an internal function that only calls the syscall), and
> conditionally defining timespec_get as a thin wrapper in the case of
> 32-bit time_t, and having a #define of __timespec_get64 to timespec_get in
> an internal header in the case of 64-bit time_t.

Ok, so more like this?

#if __TIMESIZE == 64
# define timespec_get __timespec_get64
#else
# define timespec_get __timespec_get32
#endif

/* Set TS to calendar time based in time base BASE.  */
int
__timespec_get64 (struct timespec *ts, int base)
{
  switch (base)
    {
      int res;
      INTERNAL_SYSCALL_DECL (err);
    case TIME_UTC:
#ifdef __ASSUME_TIME64_SYSCALLS
  res = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
#else
# ifdef __NR_clock_gettime64
  res = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
# endif /* __NR_clock_gettime64 */
  struct timespec ts32;

  if (! in_time_t_range (ts->tv_sec))
    {
      __set_errno (EOVERFLOW);
      return -1;
    }

  valid_timespec64_to_timespec (ts, &ts32);
  res = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, &ts32);

  if (res == 0 || !INTERNAL_SYSCALL_ERROR_P (res, err))
    {
      ts->tv_sec = ts32.tv_sec;
      ts->tv_nsec = ts32.tv_nsec;
    }
#endif

      if (INTERNAL_SYSCALL_ERROR_P (res, err))
        return 0;
      break;

    default:
      return 0;
    }

  return base;
}

#if __TIMESIZE != 64
int
__timespec_get32 (struct timespec *ts, int base)
{
  struct __timespec64 ts64;

  ret = __timespec_get64 (ts64, base);

  if (res == 0 || !INTERNAL_SYSCALL_ERROR_P (res, err))
    {
      ts->tv_sec = ts64.tv_sec;
      ts->tv_nsec = ts64.tv_nsec;
    }

  return ret;
}
#endif

>
> > int
> > __timespec_get64 (struct __timespec64 *ts, int base)
> > {
> > #ifdef __ASSUME_TIME64_SYSCALLS
> >   return INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
> > #else
>
> You need
>
> # ifndef __NR_clock_gettime64
> #  define __NR_clock_gettime64 __NR_clock_gettime
> # endif
>
> here, because 64-bit platforms define __ASSUME_TIME64_SYSCALLS but with
> unsuffixed syscall names.

The kernel defines 64 suffixed syscalls, is this required because
older kernels don't do this?

>
> >   long int ret;
> > # ifdef __NR_clock_gettime64
> >   ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
> >
> >   if (ret == 0 || errno != ENOSYS)
> >     {
> >       return ret;
> >     }
>
> You need to check INTERNAL_SYSCALL_ERRNO (...) not errno; errno isn't set
> by INTERNAL_*, only by INLINE_*.

Fixed! Thanks

>
> No braces when only a single statement is inside them.
>
> >   ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, &ts32);
> >
> >   if ((ret == 0 || errno != ENOSYS) && ts)
>
> No need to consider ENOSYS here.  NULL ts isn't valid so no need to check
> for it being non-NULL.

Fixed

>
> > /* Set TS to calendar time based in time base BASE.  */
> > int
> > timespec_get (struct timespec *ts, int base)
> > {
> >   switch (base)
> >     {
> >       int res;
> >       INTERNAL_SYSCALL_DECL (err);
> >     case TIME_UTC:
> >       res = __timespec_get (ts, base);
> >       if (INTERNAL_SYSCALL_ERROR_P (res, err))
> >         return 0;
>
> This wrapper is using an uninitialized variable err.

Good point, fixed.

Alistair

>
> --
> Joseph S. Myers
> joseph@codesourcery.com