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

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

[Lukasz Majewski]

Those functions allow easy conversion between Y2038 safe struct
__timespec64 and other time related data structures (like struct timeval).

* include/time.h (valid_timeval_to_timespec64): Add.
* include/time.h (valid_timespec_to_timespec64): Likewise.
* include/time.h (valid_timespec64_to_timespec): Likewise.
* include/time.h (valid_timespec64_to_timeval): Likewise.
* include/time.h (IS_VALID_NANOSECONDS): Likewise.
* include/time.h (timespec_to_timespec64): Likewise.
* include/time.h (timespec64_to_timespec): Likewise.
* include/time.h (timespec64_to_timeval): Likewise.

---
Changes for v8:
- None

Changes for v7:
- None

Changes for v6:
- Remove the #ifdef guard on __ASSUME_TIME64_SYSCALLS as those functions
  may be needed for fallback execution paths (on e.g. __clock_settime64).

Changes for v5:
- This code is now only compiled in when __ASSUME_TIME64_SYSCALLS is NOT
  defined. Previously it was depending on #if __TIMESIZE != 64.

Changes for v4:
- None

Changes for v3:
- Remove misleading comments regarding clearing tv_pad values during
  conversion (as Linux kernel on its own ignores upper 32 bits of tv_nsec).

Changes for v3:
- Remove timespec64_clear_padding function - as kernel ignores upper 32
  bits of tv_nsec when passed via syscall to the Linux kernel

Changes for v2:
- Add timespec64_clear_padding function
---
 include/time.h | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/include/time.h b/include/time.h
index 9792948744..64b5468115 100644
--- a/include/time.h
+++ b/include/time.h
@@ -178,5 +178,88 @@ in_time_t_range (__time64_t t)
   return s == t;
 }
 
+/* Convert a known valid struct timeval into a struct __timespec64.  */
+static inline void
+valid_timeval_to_timespec64 (const struct timeval *tv32,
+			     struct __timespec64 *ts64)
+{
+  ts64->tv_sec = tv32->tv_sec;
+  ts64->tv_nsec = tv32->tv_usec * 1000;
+}
+
+/* Convert a known valid struct timespec into a struct __timespec64.  */
+static inline void
+valid_timespec_to_timespec64 (const struct timespec *ts32,
+			      struct __timespec64 *ts64)
+{
+  ts64->tv_sec = ts32->tv_sec;
+  ts64->tv_nsec = ts32->tv_nsec;
+}
+
+/* Convert a known valid struct __timespec64 into a struct timespec.  */
+static inline void
+valid_timespec64_to_timespec (const struct __timespec64 *ts64,
+			      struct timespec *ts32)
+{
+  ts32->tv_sec = (time_t) ts64->tv_sec;
+  ts32->tv_nsec = ts64->tv_nsec;
+}
+
+/* Convert a known valid struct __timespec64 into a struct timeval.  */
+static inline void
+valid_timespec64_to_timeval (const struct __timespec64 *ts64,
+			     struct timeval *tv32)
+{
+  tv32->tv_sec = (time_t) ts64->tv_sec;
+  tv32->tv_usec = ts64->tv_nsec / 1000;
+}
+
+/* Check if a value lies with the valid nanoseconds range.  */
+#define IS_VALID_NANOSECONDS(ns) ((ns) >= 0 && (ns) <= 999999999)
+
+/* Check and convert a struct timespec into a struct __timespec64.  */
+static inline bool
+timespec_to_timespec64 (const struct timespec *ts32,
+                        struct __timespec64 *ts64)
+{
+  /* Check that ts32 holds a valid count of nanoseconds.  */
+  if (! IS_VALID_NANOSECONDS (ts32->tv_nsec))
+    return false;
+  /* All ts32 fields can fit in ts64, so copy them.  */
+  valid_timespec_to_timespec64 (ts32, ts64);
+  return true;
+}
+
+/* Check and convert a struct __timespec64 into a struct timespec.  */
+static inline bool
+timespec64_to_timespec (const struct __timespec64 *ts64,
+                        struct timespec *ts32)
+{
+  /* Check that tv_nsec holds a valid count of nanoseconds.  */
+  if (! IS_VALID_NANOSECONDS (ts64->tv_nsec))
+    return false;
+  /* Check that tv_sec can fit in a __time_t.  */
+  if (! in_time_t_range (ts64->tv_sec))
+    return false;
+  /* All ts64 fields can fit in ts32, so copy them.  */
+  valid_timespec64_to_timespec (ts64, ts32);
+  return true;
+}
+
+/* Check and convert a struct __timespec64 into a struct timeval.  */
+static inline bool
+timespec64_to_timeval (const struct __timespec64 *ts64,
+                       struct timeval *tv32)
+{
+  /* Check that tv_nsec holds a valid count of nanoseconds.  */
+  if (! IS_VALID_NANOSECONDS (ts64->tv_nsec))
+    return false;
+  /* Check that tv_sec can fit in a __time_t.  */
+  if (! in_time_t_range (ts64->tv_sec))
+    return false;
+  /* All ts64 fields can fit in tv32, so copy them.  */
+  valid_timespec64_to_timeval (ts64, tv32);
+  return true;
+}
 #endif
 #endif
-- 
2.20.1

[PATCH v8 0/3] y2038: Linux: Introduce __clock_settime64 function

[Lukasz Majewski]

This patch set introduces the conversion of clock_settime to explicit
64 bit struct __timespec64 arguments. As a result this function is now
Y2038 safe.

This work is (loosely) based on a previous development/patches:
https://libc-alpha.sourceware.narkive.com/zniMOWui/rfc-patch-00-52-make-glibc-y2038-proof#post68

Github branch (including the y2038 conversion example):
https://github.com/lmajewski/y2038_glibc/commits/glibc__clock_settime-conversion-v8

Those patches have been applied on top of master branch:
SHA1: 87accae3978c77c1a50d19ea8e3da3f0248d2612

Shall be used with provided meta-y2038 for development and testing:
https://github.com/lmajewski/meta-y2038

I've used guidelines from:
https://www.gnu.org/software/libc/manual/html_mono/libc.html
"D.2.1 64-bit time symbol handling in the GNU C Library"
to convert *clock_settime*.

and most notably from:
https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#clock_gettime.28.29


Lukasz Majewski (3):
  y2038: Introduce internal for glibc struct __timespec64
  y2038: Provide conversion helpers for struct __timespec64
  y2038: linux: Provide __clock_settime64 implementation

 include/time.h                          | 115 ++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/clock_settime.c |  38 +++++++-
 2 files changed, 149 insertions(+), 4 deletions(-)

-- 
2.20.1

[PATCH v8 3/3] y2038: linux: Provide __clock_settime64 implementation

[Lukasz Majewski]

This patch provides new __clock_settime64 explicit 64 bit function for
setting the time. Moreover, a 32 bit version - __clock_settime - has been
refactored to internally use __clock_settime64.

The __clock_settime is now supposed to be used on systems still supporting
32 bit time (__TIMESIZE != 64) - hence the necessary conversion to 64 bit
struct timespec.

The new clock_settime64 syscall available from Linux 5.1+ has been used,
when applicable.

In this patch the internal padding (tv_pad) of struct __timespec64 is
left untouched (on systems with __WORDSIZE == 32) as Linux kernel ignores
upper 32 bits of tv_nsec.

Tests:
- The code has been tested with x86_64/x86 (native compilation):
make PARALLELMFLAGS="-j8" && make xcheck PARALLELMFLAGS="-j8"

- 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
on kernels with and without 64 bit time support.

No regressions were observed.

* include/time.h (__clock_settime64):
  Add __clock_settime alias according to __TIMESIZE define
* sysdeps/unix/sysv/linux/clock_settime.c (__clock_settime):
  Refactor this function to be used only on 32 bit machines as a wrapper
  on __clock_settime64.
* sysdeps/unix/sysv/linux/clock_settime.c (__clock_settime64): Add
* sysdeps/unix/sysv/linux/clock_settime.c (__clock_settime64):
  Use clock_settime64 kernel syscall (available from 5.1+ Linux)


---
Changes for v8:
- None

Changes for v7:
- Revert changes to __clock_settime - as a result it now behaves as in v5
  (It is a wrapper on __clock_settime64).
- Fix the code to apply on newest master (after moving clock_settime to
  libc from librt).

Changes for v6:
- In the __clock_settime function do not call __clock_settime64 - just use
  the clock_settime 32 bit ABI syscall. Such approach will facilitate
  updating systems with __WORDSIZE==32 to be Y2038 safe by disabling for
  example clock_settime 32 bit syscall in the Linux kernel.

Changes for v5:
- Use __ASSUME_TIME64_SYSCALLS to indicate Linux kernel support for 64 bit
  time.
- Move the in_time_t_range() check to __clock_settime64
- Alias __NR_clock_settime64 to __NR_clock_settime if the former is not
  defined in the headers.

Changes for v4:
- __ASSUME_TIME64_SYSCALLS for fall back path
- Use __SYSCALL_WORDSIZE to exclude 'x32' from execution path (so it will
  use x86_64 syscall
- Rewrite the commit message

Changes for v3:
- Rename __ASSUME_64BIT_TIME to __ASSUME_TIME64_SYSCALLS
- Refactor in-code comment (add information regarding Linux kernel ignorance
  of padding
- Do not use __TIMESIZE to select main execution path (for Y2038 systems
  __TIMESIZE would be changed from 32 to 64 bits at some point to indicate
  full Y2038 support

Changes for v2:
- Add support for __ASSUME_64BIT_TIME flag when Linux kernel provides syscalls
  supporting 64 bit time on 32 bit systems
- Provide fallback to 32 bit version of clock_settime when clock_settime64
  is not available
- Do not copy *tp to timespec - this seems like an overkill as in clock_settime()
  the 32 bit struct timespec is copied to internal 64 bit struct __timespec64

# Conflicts:
#	sysdeps/unix/sysv/linux/clock_settime.c
---
 include/time.h                          |  8 ++++++
 sysdeps/unix/sysv/linux/clock_settime.c | 38 ++++++++++++++++++++++---
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/include/time.h b/include/time.h
index 64b5468115..91f6280eb4 100644
--- a/include/time.h
+++ b/include/time.h
@@ -124,6 +124,14 @@ extern __time64_t __timegm64 (struct tm *__tp) __THROW;
 libc_hidden_proto (__timegm64)
 #endif
 
+#if __TIMESIZE == 64
+# define __clock_settime64 __clock_settime
+#else
+extern int __clock_settime64 (clockid_t clock_id,
+                              const struct __timespec64 *tp);
+libc_hidden_proto (__clock_settime64)
+#endif
+
 /* Compute the `struct tm' representation of T,
    offset OFFSET seconds east of UTC,
    and store year, yday, mon, mday, wday, hour, min, sec into *TP.
diff --git a/sysdeps/unix/sysv/linux/clock_settime.c b/sysdeps/unix/sysv/linux/clock_settime.c
index f76178e0f6..1295bb5603 100644
--- a/sysdeps/unix/sysv/linux/clock_settime.c
+++ b/sysdeps/unix/sysv/linux/clock_settime.c
@@ -20,11 +20,9 @@
 #include <time.h>
 #include <shlib-compat.h>
 
-#include "kernel-posix-cpu-timers.h"
-
 /* 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)
@@ -33,8 +31,40 @@ __clock_settime (clockid_t clock_id, const struct timespec *tp)
       return -1;
     }
 
-  return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp);
+#ifdef __ASSUME_TIME64_SYSCALLS
+# ifndef __NR_clock_settime64
+#  define __NR_clock_settime64 __NR_clock_settime
+# endif
+  return INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
+#else
+# ifdef __NR_clock_settime64
+  int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
+  if (ret == 0 || errno != ENOSYS)
+    return ret;
+# endif
+  if (! in_time_t_range (tp->tv_sec))
+    {
+      __set_errno (EOVERFLOW);
+      return -1;
+    }
+
+  struct timespec ts32;
+  valid_timespec64_to_timespec (tp, &ts32);
+  return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);
+#endif
 }
+
+#if __TIMESIZE != 64
+int
+__clock_settime (clockid_t clock_id, const struct timespec *tp)
+{
+  struct __timespec64 ts64;
+
+  valid_timespec_to_timespec64 (tp, &ts64);
+  return __clock_settime64 (clock_id, &ts64);
+}
+#endif
+
 libc_hidden_def (__clock_settime)
 
 versioned_symbol (libc, __clock_settime, clock_settime, GLIBC_2_17);
-- 
2.20.1

[PATCH v8 1/3] y2038: Introduce internal for glibc struct __timespec64

[Lukasz Majewski]

This type is a glibc's "internal" type similar to struct timespec but
whose tv_sec field is a __time64_t rather than a time_t, which makes it
Y2038-proof and usable to pass syscalls between user code and Y2038-proof
kernel.

To support passing this structure to the kernel - the tv_pad, 32 bit int,
has been introduced. The placement of it depends on endianness of the SoC.

Tested on x86_64 and ARM.

* include/time.h: Add struct __timespec64 definition

---
Changes for v8:
- Replace #if __WORDSIZE == 64 \
           || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
  condition with #if __TIMESIZE == 64
  to allow usage of struct __timespec64 aliased to timespec on ports
  supporting 64 bit time API from the outset.

  Architectures with __TIMESIZE == 32 will use struct __timespec64 with
  __time64_t tv_sec and __int32_t tv_pad and tv_nsec.

Changes for v7:
- None

Changes for v6:
- None

Changes for v5:
- Reword commit message
- Add missing preprocessor condition for x32 (to use timespec instead of
  __timespec64).

Changes for v4:
- Change tv_pad's type from named bitfield to 32 bit int

Changes for v3:
- Replace __TIMESIZE with __WORDSIZE (as architectures with __TIMESIZE==64
  will need to use this struct with 32 bit tv_nsec field).
- Update in-code comment

Changes for v2:
- None
---
 include/time.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/time.h b/include/time.h
index dcf91855ad..9792948744 100644
--- a/include/time.h
+++ b/include/time.h
@@ -5,6 +5,7 @@
 # include <bits/types/locale_t.h>
 # include <stdbool.h>
 # include <time/mktime-internal.h>
+# include <endian.h>
 
 extern __typeof (strftime_l) __strftime_l;
 libc_hidden_proto (__strftime_l)
@@ -49,6 +50,29 @@ extern void __tzset_parse_tz (const char *tz) attribute_hidden;
 extern void __tz_compute (__time64_t timer, struct tm *tm, int use_localtime)
   __THROW attribute_hidden;
 
+#if __TIMESIZE == 64
+# define __timespec64 timespec
+#else
+/* The glibc Y2038-proof struct __timespec64 structure for a time value.
+   To keep things Posix-ish, we keep the nanoseconds field a 32-bit
+   signed long, but since the Linux field is a 64-bit signed int, we
+   pad our tv_nsec with a 32-bit int.
+
+   As a general rule the Linux kernel is ignoring upper 32 bits of
+   tv_nsec field.  */
+struct __timespec64
+{
+  __time64_t tv_sec;         /* Seconds */
+# if BYTE_ORDER == BIG_ENDIAN
+  __int32_t tv_pad;          /* Padding */
+  __int32_t tv_nsec;         /* Nanoseconds */
+# else
+  __int32_t tv_nsec;         /* Nanoseconds */
+  __int32_t tv_pad;          /* Padding */
+# endif
+};
+#endif
+
 #if __TIMESIZE == 64
 # define __ctime64 ctime
 #else
-- 
2.20.1

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

[Joseph Myers]

On Wed, 18 Sep 2019, Lukasz Majewski wrote:

> Tests:
> - The code has been tested with x86_64/x86 (native compilation):
> make PARALLELMFLAGS="-j8" && make xcheck PARALLELMFLAGS="-j8"
> 
> - 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
> on kernels with and without 64 bit time support.

Could you give more details of the configurations (including kernel 
headers version, --enable-kernel version, kernel version used at runtime) 
for which you have built glibc and run the full glibc testsuite?  I 
suspect this code is pretty close to being ready to go in - but such 
patches have plenty of scope for mistakes (such as were in earlier RV32 
patch versions) such as typos, transposed parameters, etc., that are hard 
for humans to spot reliably and easy for compilers and testsuites to spot, 
so it's important to know that sufficient automated tests have been run to 
catch any such mistakes.

I gave a list in 
<https://sourceware.org/ml/libc-alpha/2019-08/msg00234.html> of five 
configurations I think are relevant to cover the different cases in 
patches such as this ("new" = 5.1 or later; Florian's changes to provide 
syscall tables in glibc would allow "#ifdef __NR_clock_settime64" to be 
removed and eliminate case (b)):

(a) one that has always had 64-bit time, e.g. x86_64;

(b) one with 32-bit time and old kernel headers (any kernel version at 
runtime);

(c) one with 32-bit time and new kernel headers, old kernel at runtime;

(d) one with 32-bit time and new kernel headers, new kernel at runtime but 
no --enable-kernel;

(e) one with 32-bit time and new kernel at runtime and new 
--enable-kernel.

If some of these are problematic to test, you can ask for help testing 
them.  For *this particular* patch you might not need to test both (c) and 
(d), because they are identical as far as compilation is concerned and the 
testsuite doesn't really cover execution of clock_settime anyway.  But 
it's in the nature of the Y2038 changes - involving lots of conditional 
code - that it's necessary to test in several different configurations to 
cover the conditionals adequately.

(For avoidance of doubt, it is *not* necessary to run build-many-glibcs.py 
for this patch, and nor would running build-many-glibcs.py be sufficient 
since it doesn't do execution testing or cover the kernel headers version 
and --enable-kernel variants.)

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Lukasz Majewski]

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

Hi Joseph,

> On Wed, 18 Sep 2019, Lukasz Majewski wrote:
> 
> > Tests:
> > - The code has been tested with x86_64/x86 (native compilation):
> > make PARALLELMFLAGS="-j8" && make xcheck PARALLELMFLAGS="-j8"
> > 
> > - 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
> > on kernels with and without 64 bit time support.  
> 
> Could you give more details of the configurations (including kernel 
> headers version, --enable-kernel version, kernel version used at
> runtime) for which you have built glibc and run the full glibc
> testsuite?  I suspect this code is pretty close to being ready to go
> in - but such patches have plenty of scope for mistakes (such as were
> in earlier RV32 patch versions) such as typos, transposed parameters,
> etc., that are hard for humans to spot reliably and easy for
> compilers and testsuites to spot, so it's important to know that
> sufficient automated tests have been run to catch any such mistakes.
> 

So I tested this on ARM32 QEMU BSP build with meta-y2038 [1].

I. Build testing:
- Machine's
MACHINE=qemux86-64-x32
MACHINE=qemux86-64
MACHINE=qemux86
MACHINE=qemuarm

- Using glib's
test-wrapper='/opt/Y2038/glibc/src/scripts/cross-test-ssh.sh

for qemu ARM.

- ../src/scripts/build-many-glibcs.py


II. Runtime testing - with BSP [1]:
runqemu -d y2038arm nographic

- PREFERRED_VERSION_linux-y2038 = "5.1%" &&
  Y2038_GLIBC_MIN_KERNEL_VERSION="5.1.0"
  --enable-kernel=${Y2038_GLIBC_MIN_KERNEL_VERSION}

  Linux y2038arm 5.1.21-y2038-4a9b1eb8bc3ba4ad8b3b1aa3317cf8d4a3aaad83

  (Support __ASSUME_TIME64_SYSCALLS defined)

- Only PREFERRED_VERSION_linux-y2038 = "5.1%" - the minimal kernel
  version is default one for current mainline glibc

  (The __ASSUME_TIME64_SYSCALLS not defined, but kernel supports
  __clock_settime64 syscalls).

- PREFERRED_VERSION_linux-lts = "4.19%"
  with default minimal kernel version for contemporary glibc
  (SHA1: 87accae3978c77c1a50d19ea8e3da3f0248d2612)

  This kernel doesn't support __clock_settime64 syscalls, so we
  fallback to clock_settime.

Above tests are performed with Y2038 redirection applied [2] as well as
without (so the __TIMESIZE != 64 execution path is checked as well).

III. Syscalls unit tests - test_y2038 program [3] installed on BSP.

IV. For x86_64 I do run the - as I can do it on my HOST PC

make PARALLELMFLAGS="-j8" && make xcheck PARALLELMFLAGS="-j8"

(before and after the patchset).

> I gave a list in 
> <https://sourceware.org/ml/libc-alpha/2019-08/msg00234.html> of five 
> configurations I think are relevant to cover the different cases in 
> patches such as this ("new" = 5.1 or later; Florian's changes to
> provide syscall tables in glibc would allow "#ifdef
> __NR_clock_settime64" to be removed and eliminate case (b)):
> 
> (a) one that has always had 64-bit time, e.g. x86_64;
> 

Point IV.

> (b) one with 32-bit time and old kernel headers (any kernel version
> at runtime);

Point II.

> 
> (c) one with 32-bit time and new kernel headers, old kernel at
> runtime;
> 
> (d) one with 32-bit time and new kernel headers, new kernel at
> runtime but no --enable-kernel;
> 
> (e) one with 32-bit time and new kernel at runtime and new 
> --enable-kernel.

Point II.

> 

I will double check if the above points are in sync with points a) to
e).

> If some of these are problematic to test, you can ask for help
> testing them.  For *this particular* patch you might not need to test
> both (c) and (d), because they are identical as far as compilation is
> concerned and the testsuite doesn't really cover execution of
> clock_settime anyway.  But it's in the nature of the Y2038 changes -
> involving lots of conditional code - that it's necessary to test in
> several different configurations to cover the conditionals adequately.

QEMU (and OE/Yocto) helps here ...

> 
> (For avoidance of doubt, it is *not* necessary to run
> build-many-glibcs.py for this patch, and nor would running
> build-many-glibcs.py be sufficient since it doesn't do execution
> testing or cover the kernel headers version and --enable-kernel
> variants.)
> 

The build-many-glibcs helps with checking if there aren't some odd
build breaks. It is one of many test approaches for testing the Y2038
problem.

Note:

[1] - https://github.com/lmajewski/meta-y2038
[2] -
https://github.com/lmajewski/y2038_glibc/commit/1229b54508d0bb130a017a5b5591209167255665
[3] - https://github.com/lmajewski/y2038-tests/commits/master

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 --]

Re: [PATCH v8 0/3] y2038: Linux: Introduce __clock_settime64 function

[Alistair Francis]

On Wed, Sep 18, 2019 at 2:16 PM Lukasz Majewski <lukma@denx.de> wrote:
>
> This patch set introduces the conversion of clock_settime to explicit
> 64 bit struct __timespec64 arguments. As a result this function is now
> Y2038 safe.
>
> This work is (loosely) based on a previous development/patches:
> https://libc-alpha.sourceware.narkive.com/zniMOWui/rfc-patch-00-52-make-glibc-y2038-proof#post68
>
> Github branch (including the y2038 conversion example):
> https://github.com/lmajewski/y2038_glibc/commits/glibc__clock_settime-conversion-v8
>
> Those patches have been applied on top of master branch:
> SHA1: 87accae3978c77c1a50d19ea8e3da3f0248d2612
>
> Shall be used with provided meta-y2038 for development and testing:
> https://github.com/lmajewski/meta-y2038
>
> I've used guidelines from:
> https://www.gnu.org/software/libc/manual/html_mono/libc.html
> "D.2.1 64-bit time symbol handling in the GNU C Library"
> to convert *clock_settime*.
>
> and most notably from:
> https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#clock_gettime.28.29

This series fixes my build failures, thanks for this Lukasz!

Alistair

>
>
> Lukasz Majewski (3):
>   y2038: Introduce internal for glibc struct __timespec64
>   y2038: Provide conversion helpers for struct __timespec64
>   y2038: linux: Provide __clock_settime64 implementation
>
>  include/time.h                          | 115 ++++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/clock_settime.c |  38 +++++++-
>  2 files changed, 149 insertions(+), 4 deletions(-)
>
> --
> 2.20.1
>

Re: [PATCH v8 0/3] y2038: Linux: Introduce __clock_settime64 function

[Lukasz Majewski]

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

Hi Alistair,

> On Wed, Sep 18, 2019 at 2:16 PM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > This patch set introduces the conversion of clock_settime to
> > explicit 64 bit struct __timespec64 arguments. As a result this
> > function is now Y2038 safe.
> >
> > This work is (loosely) based on a previous development/patches:
> > https://libc-alpha.sourceware.narkive.com/zniMOWui/rfc-patch-00-52-make-glibc-y2038-proof#post68
> >
> > Github branch (including the y2038 conversion example):
> > https://github.com/lmajewski/y2038_glibc/commits/glibc__clock_settime-conversion-v8
> >
> > Those patches have been applied on top of master branch:
> > SHA1: 87accae3978c77c1a50d19ea8e3da3f0248d2612
> >
> > Shall be used with provided meta-y2038 for development and testing:
> > https://github.com/lmajewski/meta-y2038
> >
> > I've used guidelines from:
> > https://www.gnu.org/software/libc/manual/html_mono/libc.html
> > "D.2.1 64-bit time symbol handling in the GNU C Library"
> > to convert *clock_settime*.
> >
> > and most notably from:
> > https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#clock_gettime.28.29
> >  
> 
> This series fixes my build failures, thanks for this Lukasz!

Great :-)

> 
> Alistair
> 
> >
> >
> > Lukasz Majewski (3):
> >   y2038: Introduce internal for glibc struct __timespec64
> >   y2038: Provide conversion helpers for struct __timespec64
> >   y2038: linux: Provide __clock_settime64 implementation
> >
> >  include/time.h                          | 115
> > ++++++++++++++++++++++++ sysdeps/unix/sysv/linux/clock_settime.c |
> > 38 +++++++- 2 files changed, 149 insertions(+), 4 deletions(-)
> >
> > --
> > 2.20.1
> >  


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 --]

Re: [PATCH v8 1/3] y2038: Introduce internal for glibc struct __timespec64

[Joseph Myers]

On Wed, 18 Sep 2019, Lukasz Majewski wrote:

> This type is a glibc's "internal" type similar to struct timespec but
> whose tv_sec field is a __time64_t rather than a time_t, which makes it
> Y2038-proof and usable to pass syscalls between user code and Y2038-proof
> kernel.
> 
> To support passing this structure to the kernel - the tv_pad, 32 bit int,
> has been introduced. The placement of it depends on endianness of the SoC.
> 
> Tested on x86_64 and ARM.
> 
> * include/time.h: Add struct __timespec64 definition

This patch is OK.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v8 2/3] y2038: Provide conversion helpers for struct __timespec64

[Joseph Myers]

On Wed, 18 Sep 2019, Lukasz Majewski wrote:

> +/* Convert a known valid struct __timespec64 into a struct timespec.  */

It's not just "valid", it's valid *and within range of struct timespec*.  
(That is, I think the comment needs to be expanded.)

> +/* Convert a known valid struct __timespec64 into a struct timeval.  */

Likewise.

> +/* Check and convert a struct timespec into a struct __timespec64.  */
> +static inline bool
> +timespec_to_timespec64 (const struct timespec *ts32,
> +                        struct __timespec64 *ts64)

The comment on the function needs to say what the return value means.

> +/* Check and convert a struct __timespec64 into a struct timespec.  */
> +static inline bool
> +timespec64_to_timespec (const struct __timespec64 *ts64,
> +                        struct timespec *ts32)

Likewise.

> +/* Check and convert a struct __timespec64 into a struct timeval.  */
> +static inline bool
> +timespec64_to_timeval (const struct __timespec64 *ts64,
> +                       struct timeval *tv32)

Likewise.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v8 2/3] y2038: Provide conversion helpers for struct __timespec64

[Lukasz Majewski]

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

Hi Joseph,

> On Wed, 18 Sep 2019, Lukasz Majewski wrote:
> 
> > +/* Convert a known valid struct __timespec64 into a struct
> > timespec.  */  
> 
> It's not just "valid", it's valid *and within range of struct
> timespec*. (That is, I think the comment needs to be expanded.)
> 
> > +/* Convert a known valid struct __timespec64 into a struct
> > timeval.  */  
> 
> Likewise.
> 
> > +/* Check and convert a struct timespec into a struct __timespec64.
> >  */ +static inline bool
> > +timespec_to_timespec64 (const struct timespec *ts32,
> > +                        struct __timespec64 *ts64)  
> 
> The comment on the function needs to say what the return value means.
> 
> > +/* Check and convert a struct __timespec64 into a struct timespec.
> >  */ +static inline bool
> > +timespec64_to_timespec (const struct __timespec64 *ts64,
> > +                        struct timespec *ts32)  
> 
> Likewise.
> 
> > +/* Check and convert a struct __timespec64 into a struct timeval.
> > */ +static inline bool
> > +timespec64_to_timeval (const struct __timespec64 *ts64,
> > +                       struct timeval *tv32)  
> 
> Likewise.
> 

I will wait for your review of patch 3/3 of this series ( the one which
converts clock_settime to use __clock_settime64 internally) and send v9
with adjusted comments.

Thanks for the review.

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 --]

Re: [PATCH v8 2/3] y2038: Provide conversion helpers for struct __timespec64

[Joseph Myers]

On Thu, 19 Sep 2019, Lukasz Majewski wrote:

> I will wait for your review of patch 3/3 of this series ( the one which
> converts clock_settime to use __clock_settime64 internally) and send v9
> with adjusted comments.

Please send the new version (after committing patch 1) without waiting for 
such a review.  (At least the proposed commit message for patch 3 should 
be revised to give more details of the configurations in which it was 
tested, even if you don't have any changes for the patch itself at this 
time.)

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Lukasz Majewski]

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

Hi Joseph,

> Hi Joseph,
> 
> > On Wed, 18 Sep 2019, Lukasz Majewski wrote:
> >   
> > > Tests:
> > > - The code has been tested with x86_64/x86 (native compilation):
> > > make PARALLELMFLAGS="-j8" && make xcheck PARALLELMFLAGS="-j8"
> > > 
> > > - 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
> > > on kernels with and without 64 bit time support.    
> > 
> > Could you give more details of the configurations (including kernel 
> > headers version, --enable-kernel version, kernel version used at
> > runtime) for which you have built glibc and run the full glibc
> > testsuite?  I suspect this code is pretty close to being ready to go
> > in - but such patches have plenty of scope for mistakes (such as
> > were in earlier RV32 patch versions) such as typos, transposed
> > parameters, etc., that are hard for humans to spot reliably and
> > easy for compilers and testsuites to spot, so it's important to
> > know that sufficient automated tests have been run to catch any
> > such mistakes. 
> 
> So I tested this on ARM32 QEMU BSP build with meta-y2038 [1].
> 
> I. Build testing:
> - Machine's
> MACHINE=qemux86-64-x32
> MACHINE=qemux86-64
> MACHINE=qemux86
> MACHINE=qemuarm
> 
> - Using glib's
> test-wrapper='/opt/Y2038/glibc/src/scripts/cross-test-ssh.sh
> 
> for qemu ARM.
> 
> - ../src/scripts/build-many-glibcs.py
> 
> 
> II. Runtime testing - with BSP [1]:
> runqemu -d y2038arm nographic
> 
> - PREFERRED_VERSION_linux-y2038 = "5.1%" &&
>   Y2038_GLIBC_MIN_KERNEL_VERSION="5.1.0"
>   --enable-kernel=${Y2038_GLIBC_MIN_KERNEL_VERSION}
> 
>   Linux y2038arm 5.1.21-y2038-4a9b1eb8bc3ba4ad8b3b1aa3317cf8d4a3aaad83
> 
>   (Support __ASSUME_TIME64_SYSCALLS defined)
> 
> - Only PREFERRED_VERSION_linux-y2038 = "5.1%" - the minimal kernel
>   version is default one for current mainline glibc
> 
>   (The __ASSUME_TIME64_SYSCALLS not defined, but kernel supports
>   __clock_settime64 syscalls).
> 
> - PREFERRED_VERSION_linux-lts = "4.19%"
>   with default minimal kernel version for contemporary glibc
>   (SHA1: 87accae3978c77c1a50d19ea8e3da3f0248d2612)
> 
>   This kernel doesn't support __clock_settime64 syscalls, so we
>   fallback to clock_settime.
> 
> Above tests are performed with Y2038 redirection applied [2] as well
> as without (so the __TIMESIZE != 64 execution path is checked as
> well).
> 
> III. Syscalls unit tests - test_y2038 program [3] installed on BSP.
> 
> IV. For x86_64 I do run the - as I can do it on my HOST PC
> 
> make PARALLELMFLAGS="-j8" && make xcheck PARALLELMFLAGS="-j8"
> 
> (before and after the patchset).
> 
> > I gave a list in 
> > <https://sourceware.org/ml/libc-alpha/2019-08/msg00234.html> of
> > five configurations I think are relevant to cover the different
> > cases in patches such as this ("new" = 5.1 or later; Florian's
> > changes to provide syscall tables in glibc would allow "#ifdef
> > __NR_clock_settime64" to be removed and eliminate case (b)):
> > 
> > (a) one that has always had 64-bit time, e.g. x86_64;
> >   
> 
> Point IV.

For this point I've only used make && make xcheck

> 
> > (b) one with 32-bit time and old kernel headers (any kernel version
> > at runtime);  
> 
> Point II.
> 
> > 
> > (c) one with 32-bit time and new kernel headers, old kernel at
> > runtime;
> > 
> > (d) one with 32-bit time and new kernel headers, new kernel at
> > runtime but no --enable-kernel;

Point c) and d) test scenario as described:

https://github.com/lmajewski/meta-y2038


> > 
> > (e) one with 32-bit time and new kernel at runtime and new 
> > --enable-kernel.  
> 
> Point II.
> 
> >   
> 
> I will double check if the above points are in sync with points a) to
> e).
> 
> > If some of these are problematic to test, you can ask for help
> > testing them.  For *this particular* patch you might not need to
> > test both (c) and (d), because they are identical as far as
> > compilation is concerned and the testsuite doesn't really cover
> > execution of clock_settime anyway.  But it's in the nature of the
> > Y2038 changes - involving lots of conditional code - that it's
> > necessary to test in several different configurations to cover the
> > conditionals adequately.  
> 
> QEMU (and OE/Yocto) helps here ...
> 
> > 
> > (For avoidance of doubt, it is *not* necessary to run
> > build-many-glibcs.py for this patch, and nor would running
> > build-many-glibcs.py be sufficient since it doesn't do execution
> > testing or cover the kernel headers version and --enable-kernel
> > variants.)
> >   
> 
> The build-many-glibcs helps with checking if there aren't some odd
> build breaks. It is one of many test approaches for testing the Y2038
> problem.
> 
> Note:
> 
> [1] - https://github.com/lmajewski/meta-y2038
> [2] -
> https://github.com/lmajewski/y2038_glibc/commit/1229b54508d0bb130a017a5b5591209167255665
> [3] - https://github.com/lmajewski/y2038-tests/commits/master
> 
> 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




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 --]

Re: [PATCH v8 2/3] y2038: Provide conversion helpers for struct __timespec64

[Lukasz Majewski]

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

On Thu, 19 Sep 2019 21:28:50 +0000
Joseph Myers <joseph@codesourcery.com> wrote:

> On Thu, 19 Sep 2019, Lukasz Majewski wrote:
> 
> > I will wait for your review of patch 3/3 of this series ( the one
> > which converts clock_settime to use __clock_settime64 internally)
> > and send v9 with adjusted comments.  
> 
> Please send the new version (after committing patch 1) without
> waiting for such a review.  (At least the proposed commit message for
> patch 3 should be revised to give more details of the configurations
> in which it was tested, even if you don't have any changes for the
> patch itself at this time.)
> 

So if I understood your proposed workflow:

First the patch 1/3 get applied to glibc -master branch. Then on top of
it I shall send the corrected 2/3 (independently from review of 3/3).

Am I correct?


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 --]

Re: [PATCH v8 2/3] y2038: Provide conversion helpers for struct __timespec64

[Joseph Myers]

On Fri, 20 Sep 2019, Lukasz Majewski wrote:

> So if I understood your proposed workflow:
> 
> First the patch 1/3 get applied to glibc -master branch. Then on top of
> it I shall send the corrected 2/3 (independently from review of 3/3).
> 
> Am I correct?

Yes.

In general, if a patch is approved, and it doesn't depend on any other 
unapproved patches, it should be committed (so making future revisions of 
any containing patch series smaller and easier to review) - unless there's 
some special reason to delay the commit until subsequent patches are ready 
as well.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v8 2/3] y2038: Provide conversion helpers for struct __timespec64

[Lukasz Majewski]

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

Hi Joseph,

> On Fri, 20 Sep 2019, Lukasz Majewski wrote:
> 
> > So if I understood your proposed workflow:
> > 
> > First the patch 1/3 get applied to glibc -master branch. Then on
> > top of it I shall send the corrected 2/3 (independently from review
> > of 3/3).
> > 
> > Am I correct?  
> 
> Yes.
> 
> In general, if a patch is approved, and it doesn't depend on any
> other unapproved patches, it should be committed (so making future
> revisions of any containing patch series smaller and easier to
> review) - unless there's some special reason to delay the commit
> until subsequent patches are ready as well.
> 

I see. Thanks for explanation.


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 --]

Re: [PATCH v8 1/3] y2038: Introduce internal for glibc struct __timespec64

[Lukasz Majewski]

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

Hi Joseph,

> On Wed, 18 Sep 2019, Lukasz Majewski wrote:
> 
> > This type is a glibc's "internal" type similar to struct timespec
> > but whose tv_sec field is a __time64_t rather than a time_t, which
> > makes it Y2038-proof and usable to pass syscalls between user code
> > and Y2038-proof kernel.
> > 
> > To support passing this structure to the kernel - the tv_pad, 32
> > bit int, has been introduced. The placement of it depends on
> > endianness of the SoC.
> > 
> > Tested on x86_64 and ARM.
> > 
> > * include/time.h: Add struct __timespec64 definition  
> 
> This patch is OK.
> 

If I may ask - are there any issues with pulling this patch to glibc
-master branch?


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: 484 bytes --]

Re: [PATCH v8 1/3] y2038: Introduce internal for glibc struct __timespec64

[Joseph Myers]

On Mon, 23 Sep 2019, Lukasz Majewski wrote:

> > > * include/time.h: Add struct __timespec64 definition  
> > 
> > This patch is OK.
> > 
> 
> If I may ask - are there any issues with pulling this patch to glibc
> -master branch?

Unless there have been other concerns expressed about this patch in 
previous discussions, and unless you've discovered any problems with it 
yourself, I'm expecting you to commit it to master.  And that's generally 
the case for most patches - if someone has explicitly judged it OK for 
inclusion and there have been no other concerns expressed about it, it 
should be committed (unless in a release freeze period or it depends on 
some uncommitted patch, in which case the commit needs to be delayed).

I think it's generally for reviewers to say if their view is "I think this 
patch is OK but we should allow more time for other people to comment", 
rather than expecting patch contributors to judge when they need to wait 
further after a patch approval.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v8 1/3] y2038: Introduce internal for glibc struct __timespec64

[Lukasz Majewski]

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

Hi Joseph,

> On Mon, 23 Sep 2019, Lukasz Majewski wrote:
> 
> > > > * include/time.h: Add struct __timespec64 definition    
> > > 
> > > This patch is OK.
> > >   
> > 
> > If I may ask - are there any issues with pulling this patch to glibc
> > -master branch?  
> 
> Unless there have been other concerns expressed about this patch in 
> previous discussions, and unless you've discovered any problems with
> it yourself, I'm expecting you to commit it to master. 

Thanks for clarification - I thought that glibc follows kernel workflow
and maintainers of respective parts (or those who do the review) apply
patches.

> And that's
> generally the case for most patches - if someone has explicitly
> judged it OK for inclusion and there have been no other concerns
> expressed about it, it should be committed (unless in a release
> freeze period or it depends on some uncommitted patch, in which case
> the commit needs to be delayed).

Ok.

> 
> I think it's generally for reviewers to say if their view is "I think
> this patch is OK but we should allow more time for other people to
> comment", rather than expecting patch contributors to judge when they
> need to wait further after a patch approval.

Yes. I do understand.

If I may ask - what is the "acceptable" time for other people from
community to jump in and comment the patch before it shall be
applied?

Is it one week or more/less ?

> 

Thanks for your help.


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 --]

Re: [PATCH v8 1/3] y2038: Introduce internal for glibc struct __timespec64

[Florian Weimer]

* Lukasz Majewski:

> This type is a glibc's "internal" type similar to struct timespec but
> whose tv_sec field is a __time64_t rather than a time_t, which makes it
> Y2038-proof and usable to pass syscalls between user code and Y2038-proof
> kernel.
>
> To support passing this structure to the kernel - the tv_pad, 32 bit int,
> has been introduced. The placement of it depends on endianness of the SoC.
>
> Tested on x86_64 and ARM.
>
> * include/time.h: Add struct __timespec64 definition

Please fix the subject before committing.  A word seems to be missing
after “internal”.  Thanks.

Re: [PATCH v8 1/3] y2038: Introduce internal for glibc struct __timespec64

[Florian Weimer]

* Lukasz Majewski:

>> I think it's generally for reviewers to say if their view is "I think
>> this patch is OK but we should allow more time for other people to
>> comment", rather than expecting patch contributors to judge when they
>> need to wait further after a patch approval.
>
> Yes. I do understand.
>
> If I may ask - what is the "acceptable" time for other people from
> community to jump in and comment the patch before it shall be
> applied?
>
> Is it one week or more/less ?

A week is more than enough, especially for patches that only touch
internals like this one.

Regarding the actual patch, I don't understand why tv_pad isn't an
*anonymous* bit field.  This seems to introduce unnecessary variance
between architectures and is incompatible with how glibc itself uses
struct timespec.  It's also inconsistent with the new comment in
include/time.h (named padding is only needed if you need to
zero-initialize the padding).

Re: [PATCH v8 1/3] y2038: Introduce internal for glibc struct __timespec64

[Lukasz Majewski]

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

Hi Florian,

> * Lukasz Majewski:
> 
> > This type is a glibc's "internal" type similar to struct timespec
> > but whose tv_sec field is a __time64_t rather than a time_t, which
> > makes it Y2038-proof and usable to pass syscalls between user code
> > and Y2038-proof kernel.
> >
> > To support passing this structure to the kernel - the tv_pad, 32
> > bit int, has been introduced. The placement of it depends on
> > endianness of the SoC.
> >
> > Tested on x86_64 and ARM.
> >
> > * include/time.h: Add struct __timespec64 definition  
> 
> Please fix the subject before committing.  A word seems to be missing
> after “internal”.  Thanks.

Is the below version more acceptable?

"y2038: Introduce struct __timespec64 - new internal glibc type"


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 --]

Re: [PATCH v8 1/3] y2038: Introduce internal for glibc struct __timespec64

[Florian Weimer]

* Lukasz Majewski:

>> Please fix the subject before committing.  A word seems to be missing
>> after “internal”.  Thanks.
>
> Is the below version more acceptable?
>
> "y2038: Introduce struct __timespec64 - new internal glibc type"

Yes, looks fine to me.

Re: [PATCH v8 1/3] y2038: Introduce internal for glibc struct __timespec64

[Lukasz Majewski]

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

Hi Florian,

> * Lukasz Majewski:
> 
> >> I think it's generally for reviewers to say if their view is "I
> >> think this patch is OK but we should allow more time for other
> >> people to comment", rather than expecting patch contributors to
> >> judge when they need to wait further after a patch approval.  
> >
> > Yes. I do understand.
> >
> > If I may ask - what is the "acceptable" time for other people from
> > community to jump in and comment the patch before it shall be
> > applied?
> >
> > Is it one week or more/less ?  
> 
> A week is more than enough, especially for patches that only touch
> internals like this one.

Thanks for clarification.

> 
> Regarding the actual patch, I don't understand why tv_pad isn't an
> *anonymous* bit field. 

The reason for this is that we may need to clear this padding if we
plan to fix some issues - for example in kernel 5.1.0 - 5.1.4 there is
a bug for x32 which may require explicit clearing the padding.

> This seems to introduce unnecessary variance
> between architectures and is incompatible with how glibc itself uses
> struct timespec. 

The v3 of this patch had this field defined as anonymous padding.
However, there was strong objection for such approach [1].

> It's also inconsistent with the new comment in
> include/time.h (named padding is only needed if you need to
> zero-initialize the padding).

As explained above - some archs/kernels may require this named padding
for fixes.


Links:
[1] - https://sourceware.org/ml/libc-alpha/2019-05/msg00151.html

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 --]

Re: [PATCH v8 1/3] y2038: Introduce internal for glibc struct __timespec64

[Florian Weimer]

* Lukasz Majewski:

>> Regarding the actual patch, I don't understand why tv_pad isn't an
>> *anonymous* bit field. 
>
> The reason for this is that we may need to clear this padding if we
> plan to fix some issues - for example in kernel 5.1.0 - 5.1.4 there is
> a bug for x32 which may require explicit clearing the padding.

I think we cannot support those kernels with reasonable effort.  So
cross-architecture source compatibility with existing practices is
more important.

Furthermore, I think the consensus for the public struct timespec64 is
that it should use an unnamed bitfield because of the prevalence of
incorrect (according to POSIX) initializers.

>> This seems to introduce unnecessary variance
>> between architectures and is incompatible with how glibc itself uses
>> struct timespec. 
>
> The v3 of this patch had this field defined as anonymous padding.
> However, there was strong objection for such approach [1].

> Links:
> [1] - https://sourceware.org/ml/libc-alpha/2019-05/msg00151.html

The patch has a named bitfield:

+  int tv_pad: 32;            /* Padding named for checking/setting */

As far as I can see, the discussion was about what was actually in the
patch, and not an unnamed bitfield.

Re: [PATCH v8 1/3] y2038: Introduce internal for glibc struct __timespec64

[Lukasz Majewski]

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

Hi Florian,

> * Lukasz Majewski:
> 
> >> Regarding the actual patch, I don't understand why tv_pad isn't an
> >> *anonymous* bit field.   
> >
> > The reason for this is that we may need to clear this padding if we
> > plan to fix some issues - for example in kernel 5.1.0 - 5.1.4 there
> > is a bug for x32 which may require explicit clearing the padding.  
> 
> I think we cannot support those kernels with reasonable effort.  So
> cross-architecture source compatibility with existing practices is
> more important.

I see your point.

> 
> Furthermore, I think the consensus for the public struct timespec64 is
> that it should use an unnamed bitfield because of the prevalence of
> incorrect (according to POSIX) initializers.

Yes. Correct.

> 
> >> This seems to introduce unnecessary variance
> >> between architectures and is incompatible with how glibc itself
> >> uses struct timespec.   
> >
> > The v3 of this patch had this field defined as anonymous padding.
> > However, there was strong objection for such approach [1].  
> 
> > Links:
> > [1] - https://sourceware.org/ml/libc-alpha/2019-05/msg00151.html  
> 
> The patch has a named bitfield:
> 
> +  int tv_pad: 32;            /* Padding named for checking/setting */
> 

Ach. Yes indeed - there was a _named_ bitfield in the patch. 

> As far as I can see, the discussion was about what was actually in the
> patch, and not an unnamed bitfield.

Yes, you seems to be right. Andreas objection was about the bitfield
usage.


Let's wait for Joseph's opinion if we shall replace named padding struct
members with unnamed bitfields (and drop potential support for fixing
issues, which would require explicit clearing of padding before passing
data to the kernel). 


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 --]

Re: [PATCH v8 1/3] y2038: Introduce internal for glibc struct __timespec64

[Joseph Myers]

On Wed, 25 Sep 2019, Lukasz Majewski wrote:

> Let's wait for Joseph's opinion if we shall replace named padding struct
> members with unnamed bitfields (and drop potential support for fixing
> issues, which would require explicit clearing of padding before passing
> data to the kernel). 

I'm not particularly concerned with whether this patch uses named padding, 
unnamed bit-field or 64-bit tv_nsec in the internal struct __timespec64.  
(I don't think a named bit-field would make sense there, however.)  If we 
use an unnamed bit-field now we can readily change it later to have a name 
if we decide to support clearing padding for compat syscalls for kernels 
5.1.0 to 5.1.4.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v8 1/3] y2038: Introduce internal for glibc struct __timespec64

[Lukasz Majewski]

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

Hi Joseph, Florian,

> On Wed, 25 Sep 2019, Lukasz Majewski wrote:
> 
> > Let's wait for Joseph's opinion if we shall replace named padding
> > struct members with unnamed bitfields (and drop potential support
> > for fixing issues, which would require explicit clearing of padding
> > before passing data to the kernel).   
> 
> I'm not particularly concerned with whether this patch uses named
> padding, unnamed bit-field or 64-bit tv_nsec in the internal struct
> __timespec64. (I don't think a named bit-field would make sense
> there, however.)  If we use an unnamed bit-field now we can readily
> change it later to have a name if we decide to support clearing
> padding for compat syscalls for kernels 5.1.0 to 5.1.4.
> 

Shall I assume that the consensus is to use unnamed bit-field to pad
the tv_nsec?


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 --]