Re: [PATCH v4 3/3] y2038: linux: Provide __clock_settime64 implementation

[Lukasz Majewski <lukma at denx dot de>]

Hi Stepan,

First of all - thank you for your reply.

> 20.05.2019 в 12:27:23 +0200 Lukasz Majewski написал:
> >  /* Set CLOCK to value TP.  */
> >  int
> > -__clock_settime (clockid_t clock_id, const struct timespec *tp)
> > +__clock_settime64 (clockid_t clock_id, const struct __timespec64
> > *tp) {
> >    /* Make sure the time cvalue is OK.  */
> >    if (tp->tv_nsec < 0 || tp->tv_nsec >= 1000000000)
> > @@ -32,6 +30,40 @@ __clock_settime (clockid_t clock_id, const
> > struct timespec *tp) return -1;
> >      }
> >  
> > +#if __WORDSIZE == 32
> > +# ifdef __NR_clock_settime64
> > +  int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> > +#  ifdef __ASSUME_TIME64_SYSCALLS
> > +  return ret;
> > +#  else
> > +  if (ret == 0 || errno != ENOSYS)
> > +    return ret;
> > +#  endif
> > +# endif
> > +  /* Fall back to syscall supporting 32bit struct timespec.  */
> > +# if (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE != 64)
> > +  struct timespec ts32;
> > +  valid_timespec64_to_timespec (tp, &ts32);
> > +  return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);
> > +# endif
> > +#endif
> >    return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp);
              ^^^^^ - [1]

> >  }  
> 
> This still uses __NR_clock_settime even if __ASSUME_TIME64_SYSCALLS is
> defined.  This won't even compile on newer 32-bit architectures where
> __NR_clock_settime is not defined.  valid_timespec64_to_timespec
> won't exist in these cases too.

Ach... I see your point. The issue would be when we switch to
__TIMESIZE == 64 for 32 bit systems. For that reason the "conversion
functions" from patch [2] shall be compiled always (no matter if we
have __TIMESIZE == 64 or == 32).

And some explanation - the
if (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE != 64) is to
prevent running the fallback on 'x32' - it shall execute on [1]
execution path.


> 
> And in_time_t_range check is missing for the fallback case.

Yes - I shall move the in_time_t_range() check from __clock_settime to
the fallback.

Last but not least - the [1] shall be executed only when __WORDSIZE !=
64 - the #else is missing

> 
> >  weak_alias (__clock_settime, clock_settime)
> > +
> > +#if __TIMESIZE != 64
> > +int
> > +__clock_settime (clockid_t clock_id, const struct timespec *tp)
> > +{
> > +  struct __timespec64 ts64;
> > +
> > +  if (! in_time_t_range (tp->tv_sec))
> > +    {
> > +      __set_errno (EOVERFLOW);
> > +      return -1;
> > +    }  
> 
> What is this if (false) { … } statement doing here?

Ok, the __clock_settime would receive as an argument the timespec with
32 bit tv_nsec, and tv_sec - hence this check shall be moved from there
to the fallback of __clock_settime64.


Considering the above changes - the code starts to look too much
convoluted. Hence, I would opt for the approach presented in the
__ASSUME_TIME64_SYSCALLS patch reply.


> 
> > +
> > +  valid_timespec_to_timespec64 (tp, &ts64);
> > +  return __clock_settime64 (clock_id, &ts64);
> > +}
> > +#endif  


Note:

[2] - [PATCH v4 2/3] y2038: Provide conversion helpers for struct
__timespec64




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: pgp1vhBqc2gr0.pgp
Description: OpenPGP digital signature