Re: [PATCH v2 1/2] Y2038: make __mktime_internal compatible with __time64_t

[Lukasz Majewski <lukma at denx dot de>]

Hi Paul,

> On 3/11/19 11:58 PM, Lukasz Majewski wrote:
> > The question is if there is a more suitable place than
> > include/time.h header to have fits_in_time() reused by both Glibc
> > and Gnulib?  
> 

Thanks for your patch.

> Hmm, well, on further thought, fits_in_time_t can live in
> include/time.h since only glibc will need it. Gnulib won't support
> two kinds of time_t types, so it should't need the function. (The
> attached patch renames it to "in_time_t_range" to avoid *_t
> pollution.)

Is the rewritten code correct?

+/* Check whether T fits in time_t.  */
+static inline bool
+in_time_t_range (__time64_t t)
+{
+  time_t s = t;
+  return s != t;
+}

Shouldn't we have: return s == t; ?

> 
> That being said, we will have to make a place for private .h code
> shared between glibc and Gnulib, because include/time.h can't easily
> be shared with Gnulib. I suggest using time/mktime-internal.h for this
> 

ACK/NAK for using time/mktime-internl.h shall be done by more
experienced glibc developer(s).

> Some other comments on that patch that I noticed while looking into
> this.
> 
> The patch added duplicate "#include <errno.h>" lines to time/timegm.c.

Ok.

> 
> I still don't get why we need __timelocal64. Glibc code can use
> __mktime64. User code shouldn't see that symbol.

If we don't need it for this conversion, then we should omit it. I will
add it if needed when preparing Y2038 patch set.

In the time/mktime.c there is:
weak_alias (mktime, timelocal), which makes the timelocal calls
aliases to mktime for time_t 32 and 64 bit (for Y2038 the proper
__REDIRECT will be added).

> 
> Come to think of it, user code shouldn't see __time64_t either. Yes,
> the name begins with two underscores so user code should avoid it,
> posix/bits/types.hbut putting __time64_t in /usr/include will just
> tempt users. It's easy to keep __time64_t out of /usr/include so
> let's do that.
> 

Is that the reason for removing __time64_t definition from
posix/bits/types.h ?

> The body of __mktime64 can be simplified; there's no need for locals
> of type __time64_t, time_t, and struct tm. And it should use
> in_time_t_range instead of doing it by hand.

Yes. Also there is no need to check for EOVERFLOW in the __mktime64()
function (as it operates inherently on 64 bit types).

> 
> The bodies of mktime and timegm [__TIMESIZE != 64] should not pass tp
> to __mktime64/__timegm64 directly, since *tp should not be updated if
> the result is in __time64_t range but out of time_t range. And timegm
> should use in_time_t_range instead of doing it by hand.
> 

Ok.

> The "#ifdef weak_alias" etc. lines can be removed now, since Gnulib
> does this stuff OK now.
> 
> timegm.c should include libc-config.h, not config.h, as this is the
> current Gnulib style for code shared with Gnulib.
> 
> Revised proposed patch attached.
> 

Just out of curiosity:

In the time/mktime-internal.h you added a comment regarding BeOS users
and posix time_t - do you know any :-) ?



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