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

login
register
mail settings
Submitter Lukasz Majewski
Date Oct. 18, 2019, 2:57 p.m.
Message ID <20191018145720.11706-1-lukma@denx.de>
Download mbox | patch
Permalink /patch/35145/
State New
Headers show

Comments

Lukasz Majewski - Oct. 18, 2019, 2:57 p.m.
This macro allows conversion between Y2038 safe struct __timespec64 and
struct timespec.
The struct __timespec64's tv_nsec field is checked if it is in the correct
range.
Moreover, the tv_sec is asserted if it fits into the time_t range.
When error is detected the errno is set accordingly and the function, which
uses this macro returns -1.

Tested with scripts/build-many-glibcs.py script.
---
 include/time.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
Paul Eggert - Oct. 18, 2019, 5:36 p.m.
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.

> +#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); the caller can decide what to do if it returns false.
Lukasz Majewski - Oct. 18, 2019, 8:44 p.m.
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

Patch

diff --git a/include/time.h b/include/time.h
index d93b16a781..c2b6c9b842 100644
--- a/include/time.h
+++ b/include/time.h
@@ -236,5 +236,28 @@  valid_timespec64_to_timeval (const struct __timespec64 ts64)
 
   return tv;
 }
+
+/* Check if a value lies with the valid nanoseconds range.  */
+#define IS_VALID_NANOSECONDS(ns) ((ns) >= 0 && (ns) <= 999999999)
+
+/* Check and convert a struct __timespec64 into a struct timespec.
+   This macro checks if the valid number of nanoseconds has been provided
+   by ts64 and if not the errno is set to EINVAL and -1 is returned.
+   Moreover, the number of seconds is check as well, if it is in the time_t
+   range. If not the errno is set to EOVERFLOW and -1 is returned.  */
+#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); })
+
 #endif
 #endif