From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 77863 invoked by alias); 13 Jun 2018 09:11:17 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 76456 invoked by uid 89); 13 Jun 2018 09:11:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: zimbra.cs.ucla.edu Subject: Re: [PATCH 2/2] Y2038: make __tz_convert compatible with 64-bit-time To: "Albert ARIBAUD (3ADEV)" , libc-alpha@sourceware.org References: <20180613070019.4639-1-albert.aribaud@3adev.fr> <20180613070019.4639-3-albert.aribaud@3adev.fr> From: Paul Eggert Openpgp: preference=signencrypt Message-ID: <14097bbd-ab4c-aec7-f532-23418a120b69@cs.ucla.edu> Date: Wed, 13 Jun 2018 09:11:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <20180613070019.4639-3-albert.aribaud@3adev.fr> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable X-SW-Source: 2018-06/txt/msg00293.txt.bz2 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 s= ame?=20 Wouldn't it be simpler to get rid of "internal_time_t" and use "__time64_t"= =20 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 =3D=3D NULL) > + { > + __set_errno (EINVAL); > + return NULL; > + } > + t64 =3D *t; > + return __ctime64 (&t64); > } I don't see why 64-bit platforms need two entry points. If time_t is 64 bit= s,=20 ctime and __ctime64 can be aliases, no? Also, it's a bit cleaner to declare= and=20 initialize t64 at the same time, i.e., '__time64_t t64 =3D *t;'; we can ass= ume=20 this C99ism in glibc nowadays. Similarly for __ctime64_r, etc. The comment inside __ctime64 is confusing, since it says "localtime64" but = the=20 code says "__localtime64". Also, I would not remove the comment that is ins= ide=20 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 th= e=20 code is using 64-bit times here, and we shouldn't be putting "Y2038"s all o= ver=20 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 =3D=3D NULL) > + { > + __set_errno (EINVAL); > + return NULL; > + } > + t64 =3D *t; > + return __gmtime64_r (&t64, tp); > } __gmtime_r sets errno=3DEINVAL for a null first argument, whereas __gmtime6= 4_r=20 dumps core instead. Why is that? If it's intended, there should be a commen= t as=20 to why the two functions differ in behavior here. Similarly for __localtime= _r,=20 gmtime, localtime, and ctime. However, won't this introduce compatibility=20 issues? Wouldn't be better for the __time64_t versions to mimic the time_t = behavior? > - tz_rules[0].change =3D tz_rules[1].change =3D (time_t) -1; > + tz_rules[0].change =3D tz_rules[1].change =3D (internal_time_t) -1; The cast is just getting making maintenance harder, so I suggest changing t= his=20 to use plain "=3D -1;" at the end.