[RFC,v6,03/23] time: Add a timeval with a long tv_sec and tv_usec

  On y2038 safe 32-bit systems the Linux kernel expects itimerval to
use a 32-bit time_t, even though the other time_t's are 64-bit. To
address this let's add a timeval_long struct to be used internally.
---
 include/time.h                   | 29 +++++++++++++++++++++++++++++
 time/bits/types/struct_timeval.h |  8 ++++++++
 2 files changed, 37 insertions(+)
  

On Sun, 12 Jan 2020, Alistair Francis wrote:

> On y2038 safe 32-bit systems the Linux kernel expects itimerval to
> use a 32-bit time_t, even though the other time_t's are 64-bit. To
> address this let's add a timeval_long struct to be used internally.
> ---
>  include/time.h                   | 29 +++++++++++++++++++++++++++++
>  time/bits/types/struct_timeval.h |  8 ++++++++

bits/types/ headers are *installed* headers that should only define the 
type in question.  If there is some good reason to need __timeval_long in 
*installed* headers (i.e. a *public* interface using this type) it should 
go in bits/types/struct___timeval_long.h, otherwise it should go in a 
non-installed header.
  

On Mon, Jan 13, 2020 at 12:48 AM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Sun, 12 Jan 2020, Alistair Francis wrote:
>
> > On y2038 safe 32-bit systems the Linux kernel expects itimerval to
> > use a 32-bit time_t, even though the other time_t's are 64-bit. To
> > address this let's add a timeval_long struct to be used internally.
> > ---
> >  include/time.h                   | 29 +++++++++++++++++++++++++++++
> >  time/bits/types/struct_timeval.h |  8 ++++++++
>
> bits/types/ headers are *installed* headers that should only define the
> type in question.  If there is some good reason to need __timeval_long in
> *installed* headers (i.e. a *public* interface using this type) it should
> go in bits/types/struct___timeval_long.h, otherwise it should go in a
> non-installed header.

Ok, I don't think this should be installed. The only possible reason
installing this would be if someone was manually calling the syscalls
and wanted to use this type to do that.

Do you have a recommendation of a non installed header where it should go?

Alistair

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

Hi Alistair,

> On y2038 safe 32-bit systems the Linux kernel expects itimerval to
> use a 32-bit time_t, even though the other time_t's are 64-bit. To
> address this let's add a timeval_long
			   ^^^^^^^^^^^^^ - I'm not so seasoned glibc
			   developer, but I think that the _long suffix
			   is a bit misleading.

	Maybe it would be more readable to name it as struct
	__timeval32 ? In that way one can see from the outset that we
	operate on 32 bit values.

	Community feedback is welcome :-)

> struct to be used internally.
> ---
>  include/time.h                   | 29 +++++++++++++++++++++++++++++
>  time/bits/types/struct_timeval.h |  8 ++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/include/time.h b/include/time.h
> index e5e8246eac..201342d1ca 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -310,6 +310,35 @@ valid_timespec64_to_timeval (const struct
> __timespec64 ts64) return tv;
>  }
>  
> +/* Conversion functions for converting to/from __timeval_long
> +.  If the seconds field of a __timeval_long would
> +   overflow, they write { INT32_MAX, 999999 } to the output.  */
> +static inline struct timeval
> +valid_timeval_long_to_timeval64 (const struct __timeval_long tv)
> +{
> +  return (struct timeval) { tv.tv_sec, tv.tv_usec };
> +}
> +
> +static inline struct __timeval_long
> +valid_timeval64_to_timeval_long (const struct timeval tv64)
> +{
> +  if (__glibc_unlikely (tv64.tv_sec > (time_t) 2147483647))
> +    return (struct __timeval_long) { 2147483647, 999999};
> +  return (struct __timeval_long) { tv64.tv_sec, tv64.tv_usec };
> +}
> +
> +static inline struct timespec
> +valid_timeval_long_to_timespec (const struct __timeval_long tv)
> +{
> +  return (struct timespec) { tv.tv_sec, tv.tv_usec * 1000 };
> +}
> +
> +static inline struct __timeval_long
> +valid_timespec_to_timeval_long (const struct timespec ts)
> +{
> +  return (struct __timeval_long) { (time_t) ts.tv_sec, ts.tv_nsec /
> 1000 }; +}
> +
>  /* Check if a value is in the valid nanoseconds range. Return true if
>     it is, false otherwise.  */
>  static inline bool
> diff --git a/time/bits/types/struct_timeval.h
> b/time/bits/types/struct_timeval.h index 70394ce886..73697689cc 100644
> --- a/time/bits/types/struct_timeval.h
> +++ b/time/bits/types/struct_timeval.h
> @@ -10,4 +10,12 @@ struct timeval
>    __time_t tv_sec;		/* Seconds.  */
>    __suseconds_t tv_usec;	/* Microseconds.  */
>  };
> +
> +/* A version of 'struct timeval' with `long` time_t
> +   and suseconds_t.  */
> +struct __timeval_long
> +{
> +  long tv_sec;		/* Seconds.  */
> +  long tv_usec;		/* Microseconds.  */
> +};
>  #endif

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
  

Hi Alistair,

> On Mon, Jan 13, 2020 at 12:48 AM Joseph Myers
> <joseph@codesourcery.com> wrote:
> >
> > On Sun, 12 Jan 2020, Alistair Francis wrote:
> >  
> > > On y2038 safe 32-bit systems the Linux kernel expects itimerval to
> > > use a 32-bit time_t, even though the other time_t's are 64-bit. To
> > > address this let's add a timeval_long struct to be used
> > > internally. ---
> > >  include/time.h                   | 29
> > > +++++++++++++++++++++++++++++ time/bits/types/struct_timeval.h |
> > > 8 ++++++++  
> >
> > bits/types/ headers are *installed* headers that should only define
> > the type in question.  If there is some good reason to need
> > __timeval_long in *installed* headers (i.e. a *public* interface
> > using this type) it should go in
> > bits/types/struct___timeval_long.h, otherwise it should go in a
> > non-installed header.  
> 
> Ok, I don't think this should be installed. The only possible reason
> installing this would be if someone was manually calling the syscalls
> and wanted to use this type to do that.
> 
> Do you have a recommendation of a non installed header where it
> should go?

I'm not sure if this hint will match your use case, but similar:

struct __timespec64 and struct __itimerspec64 are defined in
./include/time.h

This is the place for glibc internal definitions (not
installed/exported).

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




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
  

On Tue, Jan 14, 2020 at 8:17 PM Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Alistair,
>
> > On y2038 safe 32-bit systems the Linux kernel expects itimerval to
> > use a 32-bit time_t, even though the other time_t's are 64-bit. To
> > address this let's add a timeval_long
>                            ^^^^^^^^^^^^^ - I'm not so seasoned glibc
>                            developer, but I think that the _long suffix
>                            is a bit misleading.
>
>         Maybe it would be more readable to name it as struct
>         __timeval32 ? In that way one can see from the outset that we
>         operate on 32 bit values.

It isn't explicitly 32-bit, it's just always a long which is why I
went with long in the name instead.

I'm happy to change the name, it doesn't really matter too much as
it's only user internally.

I'll wait to see what others think before changing it.


Alistair

>
>         Community feedback is welcome :-)
>
> > struct to be used internally.
> > ---
> >  include/time.h                   | 29 +++++++++++++++++++++++++++++
> >  time/bits/types/struct_timeval.h |  8 ++++++++
> >  2 files changed, 37 insertions(+)
> >
> > diff --git a/include/time.h b/include/time.h
> > index e5e8246eac..201342d1ca 100644
> > --- a/include/time.h
> > +++ b/include/time.h
> > @@ -310,6 +310,35 @@ valid_timespec64_to_timeval (const struct
> > __timespec64 ts64) return tv;
> >  }
> >
> > +/* Conversion functions for converting to/from __timeval_long
> > +.  If the seconds field of a __timeval_long would
> > +   overflow, they write { INT32_MAX, 999999 } to the output.  */
> > +static inline struct timeval
> > +valid_timeval_long_to_timeval64 (const struct __timeval_long tv)
> > +{
> > +  return (struct timeval) { tv.tv_sec, tv.tv_usec };
> > +}
> > +
> > +static inline struct __timeval_long
> > +valid_timeval64_to_timeval_long (const struct timeval tv64)
> > +{
> > +  if (__glibc_unlikely (tv64.tv_sec > (time_t) 2147483647))
> > +    return (struct __timeval_long) { 2147483647, 999999};
> > +  return (struct __timeval_long) { tv64.tv_sec, tv64.tv_usec };
> > +}
> > +
> > +static inline struct timespec
> > +valid_timeval_long_to_timespec (const struct __timeval_long tv)
> > +{
> > +  return (struct timespec) { tv.tv_sec, tv.tv_usec * 1000 };
> > +}
> > +
> > +static inline struct __timeval_long
> > +valid_timespec_to_timeval_long (const struct timespec ts)
> > +{
> > +  return (struct __timeval_long) { (time_t) ts.tv_sec, ts.tv_nsec /
> > 1000 }; +}
> > +
> >  /* Check if a value is in the valid nanoseconds range. Return true if
> >     it is, false otherwise.  */
> >  static inline bool
> > diff --git a/time/bits/types/struct_timeval.h
> > b/time/bits/types/struct_timeval.h index 70394ce886..73697689cc 100644
> > --- a/time/bits/types/struct_timeval.h
> > +++ b/time/bits/types/struct_timeval.h
> > @@ -10,4 +10,12 @@ struct timeval
> >    __time_t tv_sec;           /* Seconds.  */
> >    __suseconds_t tv_usec;     /* Microseconds.  */
> >  };
> > +
> > +/* A version of 'struct timeval' with `long` time_t
> > +   and suseconds_t.  */
> > +struct __timeval_long
> > +{
> > +  long tv_sec;               /* Seconds.  */
> > +  long tv_usec;              /* Microseconds.  */
> > +};
> >  #endif
>
> 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
  

On Wed, Jan 15, 2020 at 6:15 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, Jan 14, 2020 at 8:17 PM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > Hi Alistair,
> >
> > > On y2038 safe 32-bit systems the Linux kernel expects itimerval to
> > > use a 32-bit time_t, even though the other time_t's are 64-bit. To
> > > address this let's add a timeval_long
> >                            ^^^^^^^^^^^^^ - I'm not so seasoned glibc
> >                            developer, but I think that the _long suffix
> >                            is a bit misleading.
> >
> >         Maybe it would be more readable to name it as struct
> >         __timeval32 ? In that way one can see from the outset that we
> >         operate on 32 bit values.
>
> It isn't explicitly 32-bit, it's just always a long which is why I
> went with long in the name instead.
>
> I'm happy to change the name, it doesn't really matter too much as
> it's only user internally.
>
> I'll wait to see what others think before changing it.

I think you can look at it either way: the Your timeval_long works fine
on both 32-bit and 64-bit architectures, but you really only need it
on 32-bit ones because on 64-bit architectures (except sparc64, which
has 32-bit suseconds_t !) this is the same as the default timeval
anyway.

In the kernel we actually ended up having both: there is a
"struct __kernel_old_timeval" that is part of the uapi headers
for defining structures based on the old type with no replacement
such as rusage and itimerval, and internally there is a "struct
old_timeval32" with two "int32_t" members that is used for
providing compatibility handlers for old interfaces that have a
64-bit replacement, so the same compatibility handler can be
used on native 32-bit kernels and on 64-bit kernels running
32-bit user space.

     Arnd
  

Hi Alistair,

> On Tue, Jan 14, 2020 at 8:17 PM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > Hi Alistair,
> >  
> > > On y2038 safe 32-bit systems the Linux kernel expects itimerval to
> > > use a 32-bit time_t, even though the other time_t's are 64-bit. To
> > > address this let's add a timeval_long  
> >                            ^^^^^^^^^^^^^ - I'm not so seasoned glibc
> >                            developer, but I think that the _long
> > suffix is a bit misleading.
> >
> >         Maybe it would be more readable to name it as struct
> >         __timeval32 ? In that way one can see from the outset that
> > we operate on 32 bit values.  
> 
> It isn't explicitly 32-bit, it's just always a long which is why I
> went with long in the name instead.
> 
> I'm happy to change the name, it doesn't really matter too much as
> it's only user internally.
> 
> I'll wait to see what others think before changing it.

Yes, I also think that it would be best to wait for more input from the
community.

> 
> 
> Alistair
> 
> >
> >         Community feedback is welcome :-)
> >  
> > > struct to be used internally.
> > > ---
> > >  include/time.h                   | 29
> > > +++++++++++++++++++++++++++++ time/bits/types/struct_timeval.h |
> > > 8 ++++++++ 2 files changed, 37 insertions(+)
> > >
> > > diff --git a/include/time.h b/include/time.h
> > > index e5e8246eac..201342d1ca 100644
> > > --- a/include/time.h
> > > +++ b/include/time.h
> > > @@ -310,6 +310,35 @@ valid_timespec64_to_timeval (const struct
> > > __timespec64 ts64) return tv;
> > >  }
> > >
> > > +/* Conversion functions for converting to/from __timeval_long
> > > +.  If the seconds field of a __timeval_long would
> > > +   overflow, they write { INT32_MAX, 999999 } to the output.  */
> > > +static inline struct timeval
> > > +valid_timeval_long_to_timeval64 (const struct __timeval_long tv)
> > > +{
> > > +  return (struct timeval) { tv.tv_sec, tv.tv_usec };
> > > +}
> > > +
> > > +static inline struct __timeval_long
> > > +valid_timeval64_to_timeval_long (const struct timeval tv64)
> > > +{
> > > +  if (__glibc_unlikely (tv64.tv_sec > (time_t) 2147483647))
> > > +    return (struct __timeval_long) { 2147483647, 999999};
> > > +  return (struct __timeval_long) { tv64.tv_sec, tv64.tv_usec };
> > > +}
> > > +
> > > +static inline struct timespec
> > > +valid_timeval_long_to_timespec (const struct __timeval_long tv)
> > > +{
> > > +  return (struct timespec) { tv.tv_sec, tv.tv_usec * 1000 };
> > > +}
> > > +
> > > +static inline struct __timeval_long
> > > +valid_timespec_to_timeval_long (const struct timespec ts)
> > > +{
> > > +  return (struct __timeval_long) { (time_t) ts.tv_sec,
> > > ts.tv_nsec / 1000 }; +}
> > > +
> > >  /* Check if a value is in the valid nanoseconds range. Return
> > > true if it is, false otherwise.  */
> > >  static inline bool
> > > diff --git a/time/bits/types/struct_timeval.h
> > > b/time/bits/types/struct_timeval.h index 70394ce886..73697689cc
> > > 100644 --- a/time/bits/types/struct_timeval.h
> > > +++ b/time/bits/types/struct_timeval.h
> > > @@ -10,4 +10,12 @@ struct timeval
> > >    __time_t tv_sec;           /* Seconds.  */
> > >    __suseconds_t tv_usec;     /* Microseconds.  */
> > >  };
> > > +
> > > +/* A version of 'struct timeval' with `long` time_t
> > > +   and suseconds_t.  */
> > > +struct __timeval_long
> > > +{
> > > +  long tv_sec;               /* Seconds.  */
> > > +  long tv_usec;              /* Microseconds.  */
> > > +};
> > >  #endif  
> >
> > 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