Re: [PATCH 1/2] y2038: Helper macro to convert struct __timespec64 to struct timespec

[Lukasz Majewski <lukma at denx dot de>]

Hi Paul,

> On 10/18/19 7:57 AM, Lukasz Majewski wrote:
> 
> > +/* Check if a value lies with the valid nanoseconds range.  */
> > +#define IS_VALID_NANOSECONDS(ns) ((ns) >= 0 && (ns) <= 999999999)  
> 
> This should be a static or inline function; there's no need for the 
> excessive power of a macro here.

Ok. I can add this as an inline static function.

> 
> > +#define timespec64_to_timespec(ts64)
> >             \
> > +  ({
> >             \
> > +    if (! IS_VALID_NANOSECONDS (ts64.tv_nsec))
> >             \
> > +      {
> >             \
> > +        __set_errno (EINVAL);
> >             \
> > +        return -1;
> >             \
> > +      }
> >             \
> > +    if (! in_time_t_range (ts64.tv_sec))
> >             \
> > +      {
> >             \
> > +        __set_errno (EOVERFLOW);
> >             \
> > +        return -1;
> >             \
> > +      }
> >             \
> > +    valid_timespec64_to_timespec (ts64); })  
> 
> This macro is too confusing. Instead, if there's a need for this sort
> of thing, I suggest a static or inline function that returns true or
> false (setting errno);

My first attempt on this conversion helper was based on static inline
function. Unfortunately, such approach has the issue with __set_errno(),
which is not accessible in include/time.h (as it is defined in
include/errno.h).

Moreover, in the glibc the pattern with defining such macros is widely
used - in e.g. math/math-narrow.h or in various sysdep.h headers.

Last but not least - as Joseph pointed out in the other mail - maybe it
would be just enough in this particular case to drop the
IS_VALID_NANOSECONDS() check as this shall be done in the kernel (and
if an error is detected the syscall would fail).
The in_time_t_range() check for clock_getres is also problematic - as
it would be required to have a _really_ bad clock with tv_sec to be
overflowed.

To sum up - for the clock_getres() conversion - I do opt for using
valid_timespec64_to_timespec() (as it is already available in
include/time.h) and drop this patch (but keeping the
IS_VALID_NANOSECONDS() check in the form of static inline).

> the caller can decide what to do if it returns
> false.




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