Re: [PATCH 2/2] Y2038: make __tz_convert compatible with 64-bit-time

[Paul Eggert <eggert at cs dot ucla dot edu>]

Albert ARIBAUD (3ADEV) wrote:

> -extern void __tz_compute (time_t timer, struct tm *tm, int use_localtime)
> +extern void __tz_compute (internal_time_t timer, struct tm *tm, int use_localtime)

Why have two types "internal_time_t" and "__time64_t" that are always the same? 
Wouldn't it be simpler to get rid of "internal_time_t" and use "__time64_t" 
uniformly? If we do need two types, the need should be documented clearly.

>   /* Return a string as returned by asctime which
>      is the representation of *T in that form.  */
>   char *
> +__ctime64 (const __time64_t *t)
> +{
> +  /* Apply the same rule as ctime:
> +     make ctime64 (t) is equivalent to asctime (localtime64 (t)).  */
> +  return asctime (__localtime64 (t));
> +}
> +
> +/* The 32-bit time wrapper.  */
> +char *
>   ctime (const time_t *t)
>   {
> -  /* The C Standard says ctime (t) is equivalent to asctime (localtime (t)).
> -     In particular, ctime and asctime must yield the same pointer.  */
> -  return asctime (localtime (t));
> +  __time64_t t64;
> +  if (t == NULL)
> +    {
> +      __set_errno (EINVAL);
> +      return NULL;
> +    }
> +  t64 = *t;
> +  return __ctime64 (&t64);
>   }

I don't see why 64-bit platforms need two entry points. If time_t is 64 bits, 
ctime and __ctime64 can be aliases, no? Also, it's a bit cleaner to declare and 
initialize t64 at the same time, i.e., '__time64_t t64 = *t;'; we can assume 
this C99ism in glibc nowadays.

Similarly for __ctime64_r, etc.

The comment inside __ctime64 is confusing, since it says "localtime64" but the 
code says "__localtime64". Also, I would not remove the comment that is inside 
ctime, as removing it omits important motivation.

>   /* Return a string as returned by asctime which is the representation
> -   of *T in that form.  Reentrant version.  */
> +   of *T in that form.  Reentrant Y2038-proof version.  */
>   char *
> -ctime_r (const time_t *t, char *buf)
> +__ctime64_r (const __time64_t *t, char *buf)

There is no need to add "Y2038-proof" to the comment.  It's obvious that the 
code is using 64-bit times here, and we shouldn't be putting "Y2038"s all over 
the place in the commentary.

> +/* Return the `struct tm' representation of 64-bit-time *T
> +   in UTC, using *TP to store the result.  */
> +struct tm *
> +__gmtime64_r (const __time64_t *t, struct tm *tp)
> +{
> +  return __tz_convert (*t, 0, tp);
> +}
> +
> +/* The 32-bit-time wrapper.  */
>   struct tm *
>   __gmtime_r (const time_t *t, struct tm *tp)
>   {
> -  return __tz_convert (t, 0, tp);
> +  __time64_t t64;
> +  if (t == NULL)
> +    {
> +      __set_errno (EINVAL);
> +      return NULL;
> +    }
> +  t64 = *t;
> +  return __gmtime64_r (&t64, tp);
>   }

__gmtime_r sets errno=EINVAL for a null first argument, whereas __gmtime64_r 
dumps core instead. Why is that? If it's intended, there should be a comment as 
to why the two functions differ in behavior here. Similarly for __localtime_r, 
gmtime, localtime, and ctime. However, won't this introduce compatibility 
issues? Wouldn't be better for the __time64_t versions to mimic the time_t behavior?

> -      tz_rules[0].change = tz_rules[1].change = (time_t) -1;
> +      tz_rules[0].change = tz_rules[1].change = (internal_time_t) -1;

The cast is just getting making maintenance harder, so I suggest changing this 
to use plain "= -1;" at the end.