Alan Barrett wrote:

That should be tz_posix2time, not tz_time2posix.

Thanks, fixed in the obvious way with the attached revised patch to the tz code.

More generally, it would be nice to know what went wrong on NetBSD.

The same thing that goes wrong on any platform if one wants to portability-test the tz code by compiling it with -Dtime_tz='uintmax_t' (say): the prototypes in <time.h> which use time_t='long' (say) conflict with with the altered prototypes localtime.c wants to use internally which use time_t='uintmax_t'. Unlike most other platforms, recent versions of NetBSD declare a posix2time prototype in <time.h>, so NetBSD needs a workaround for the posix2time clash even though most platforms don't. The motivation for all this is to add support for NetBSD-style localtime_rz, mktime_z, tzalloc, and tzfree, along the lines suggested by Christos Zoulas in <http://mm.icann.org/pipermail/tz/2010-September/016435.html>. These functions have been added to NetBSD. I've found some problems with the NetBSD API and plan to propose a simpler API here soon, which is binary compatible with a subset of NetBSD's. If it works out maybe we can get it into POSIX. I'll CC: this to Christos to give him a heads-up.

In thinking about what to do with possibly modifying the tz code to have time zone values as part of the API, I reviewed the NetBSD and Android bionic APIs in this area. Here are my thoughts on the NetBSD vs Android bionic interface: * Android bionic has a function mktime_tz that works like NetBSD's tzalloc + mktime_z, and which uses a cache to work around the resulting performance problems. Android's API is easier to use for mktime but the NetBSD approach feels like a better match for the rest of the API, as it's more flexible and efficient. Also, NetBSD has localtime_rz but Android lacks this functionality. So between the two the NetBSD style seems like a better way to go. However, here are some problems I found with the NetBSD API. They're relatively minor but do need fixing. * The NetBSD API's declarations confused about 'const'. Several functions take arguments of type 'const timezone_t' but the const does not have the intended meaning: it declares the argument to be a constant pointer to non-constant data, which means the 'const' is irrelevant to the caller and is ignored by the API. The simplest fix is to remove the 'const's from the API's declarations. * The arguments of localtime_rz and mktime_z should all be restrict-qualified, for the same reason that localtime_r's arguments are restrict-qualified. * ctime and ctime_r have been deprecated by POSIX due to their security issues, and ctime_rz has the same issues, so I suggest deprecating ctime_rz in NetBSD, and the tz code should not support it. Callers can use strftime instead. * tzgetname doesn't work for time zones that have three or more abbreviations. Plus, tzgetname is redundant since callers can instead use localtime_rz + strftime %Z, an approach that works even with three or more abbreviations. Again, I suggest deprecating tzgetname in NetBSD, and the tz code should not support it. I don't know whether these NetBSD issues are something Christos can fix directly or whether we need to file NetBSD bug reports or whatever.

Christos Zoulas wrote:

I can fix those directly. Would you like to supply patches?

Thanks, but I'm afraid I'm no expert in NetBSD. I don't know how you mark interfaces as obsolete, for example. I could help review patches, though.

Christos Zoulas wrote:

just adjust the prototypes how you like them, and I will take care of the rest

Thanks. I looked into doing that and found two more issues. First, there's no need for timelocal_z. timelocal is merely an ancient (pre-C89) compatibility function, superseded by mktime. There's no need to expand it, as callers can easily use mktime_z. Let's remove it. Second and more important, strftime_z has an unnecessary timezone_t argument. This argument is present to format %Z and %z, but NetBSD can format %Z from the tm_zone member of struct tm, and similarly for %z and tm_gmtoff. The timezone_t argument was invented under the idea that a strictly conforming C application can leave tm_zone and tm_gmtoff in an undefined state. Although this is true for strftime, it need not and should not be true for a new function that we define, as we can impose the restriction that if the new function is used with %Z and %z, its struct tm argument must have filled-in values. This way, the new function will do the right thing in zones that have three or more time zone abbreviations, or three or more UTC offsets, which is relatively common. In contrast strftime_z handles only the two most-current abbreviations and offsets, and this botches historical time stamps. Let's call the new function tm_strftime instead of strftime_z. It doesn't have a time zone argument so the trailing "_z" would be misleading. The "tm_" prefix can be a mnemonic that this strftime trusts its struct tm argument; also, the "tm_" prefix is reserved by POSIX for time.h extensions so it helps to avoids namespace pollution. Similarly for strftime_lz, renaming it to tm_strftime_l. A preliminary proposed patch to NetBSD-current sources attached. Again, I don't know how NetBSD does deprecation. Also, I have not tried to compile or test this.

Guy Harris wrote:

Not all OSes can do that, however, as not all OSes with "struct tm" have "tm_zone" and "tm_gmtoff" members of that structure.

Yes, that's right, the proposal is tuned for platforms that have time zone info in struct tm. Unfortunately if a struct tm is bare-bones and lacks this info, NetBSD's strftime_z can misbehave on time zones that don't fall into the simple POSIX model, because it cannot deduce strictly from a bare-bones struct tm what the time zone or abbreviation was for a past time stamp. For example, suppose TZ='Africa/Tripoli' and a bare-bones struct tm says only 2012-11-10 01:30:00 with tm_isdst == 0. This is ambiguous: it could be EET and it could be CET, as Libya changed time zones at 02:00 and repeated that hour without observing DST either before or after the transition. In this example, NetBSD's strftime_z guesses when formatting %Z or %z, and sometimes guesses wrongly. But the proposed tm_strftime function won't guess and will do the right thing on NetBSD. (In writing the above paragraph I saw a minor infelicity in the proposed patch -- not really a bug per se, just inelegance -- and the first attached patch should fix that.) If and when tm_strftime support is added to barebones struct tm platforms, I suggest adding tm_gmtoff and tm_zone members to struct tm in the usual way that system data structures are extended. These members might be hidden (e.g., they might be called __tm_gmtoff or whatever). On platforms where it's too much trouble to extend struct tm, I suppose the tm_strftime function should simply be an alias for strftime, though it will continue to botch %Z and %z occasionally. The proposed patch also fixes a C11 compatibility bug in tzcode up through 2014f, where strftime (when compiled with -DTM_ZONE) may access a bad pointer when interpreting %Z or (when compiled with -DTM_GMTOFF) an uninitialized integer when interpreting %z, even though the C standard says the behavior must not be undefined. I forgot to document that fix in NEWS; second proposed patch attached.

On Aug 25, 12:32am, eggert@cs.ucla.edu (Paul Eggert) wrote: -- Subject: Re: NetBSD and Android bionic APIs for tz values | Paul Eggert wrote: | > A preliminary proposed patch to NetBSD-current sources attached. | | Following up on this, I implemented it for tzcode; see | | http://mm.icann.org/pipermail/tz/2014-August/021516.html | | In doing so it struck me that the proposal for new functions tm_strftime | and tm_strftime_l is overkill, and that it's better to simply fix | strftime and strftime_l to do the right thing with %z and %Z, as that's | what glibc does. I understand there's an argument that POSIX doesn't | allow this behavior, but it's not clear to me that the argument is | correct, and anyway NetBSD would be in good company when it's compatible | with tzcode and with GNU/Linux here. So attached is a a revised patch | that simply removes strftime_z/strftime_lz and fixes | strftime/strftime_l. The rest of the patch is the same as before. Thanks Paul, looks fine to me. Since we can't really bump libc (and the function removal can break things; even the const removal changes the signature for c++, but I guess that's not that important) the choices are: - remove the functions and if older code used them the code will stop working - keep the functions, but put them in compat, removing them from headers. I guess I'll bring it up and see what others have to say. Thanks, christos

On Sat, Aug 16, 2014 at 6:07 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:

In thinking about what to do with possibly modifying the tz code to have time zone values as part of the API, I reviewed the NetBSD and Android bionic APIs in this area.

Here are my thoughts on the NetBSD vs Android bionic interface:

* Android bionic has a function mktime_tz that works like NetBSD's tzalloc + mktime_z, and which uses a cache to work around the resulting performance problems. Android's API is easier to use for mktime but the NetBSD approach feels like a better match for the rest of the API, as it's more flexible and efficient. Also, NetBSD has localtime_rz but Android lacks this functionality. So between the two the NetBSD style seems like a better way to go.

also, we've removed the non-standard functions from Android for LP64 :-)

However, here are some problems I found with the NetBSD API. They're relatively minor but do need fixing.

* The NetBSD API's declarations confused about 'const'. Several functions take arguments of type 'const timezone_t' but the const does not have the intended meaning: it declares the argument to be a constant pointer to non-constant data, which means the 'const' is irrelevant to the caller and is ignored by the API. The simplest fix is to remove the 'const's from the API's declarations.

* The arguments of localtime_rz and mktime_z should all be restrict-qualified, for the same reason that localtime_r's arguments are restrict-qualified.

* ctime and ctime_r have been deprecated by POSIX due to their security issues, and ctime_rz has the same issues, so I suggest deprecating ctime_rz in NetBSD, and the tz code should not support it. Callers can use strftime instead.

* tzgetname doesn't work for time zones that have three or more abbreviations. Plus, tzgetname is redundant since callers can instead use localtime_rz + strftime %Z, an approach that works even with three or more abbreviations. Again, I suggest deprecating tzgetname in NetBSD, and the tz code should not support it.

I don't know whether these NetBSD issues are something Christos can fix directly or whether we need to file NetBSD bug reports or whatever.