[PATCH] Add support for leap seconds in zoneinfo files

[PATCH] Add support for leap seconds in zoneinfo files

[Laurent Bercot]

[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]


  Attached.

  I tried to make it as simple and musl-coding-style-compliant as possible.
  One line adds support for TZif3 (it costs nothing once we have TZif2 and
we can ignore the extensions).
  The other changes are ifdef-guarded and can be ignored with -DNO_LEAPSECONDS,
for hardcore POSIX fans. ;) But posix/ rules don't have leap seconds anyway,
only right/ ones do: if you don't like #ifdef forests, feel free to remove the
guards.

  By the way, what's the musl policy on internal sanity checks ? In __tz.c,
musl doesn't check anything after the magic numbers, so if the zoneinfo file
is bad later on, musl can crash, or worse, silently return inconsistent or
wrong results. I understand that it's impossible to escalate an error to the
user when the entry point returns a void, but wouldn't it be safer, in this
case as well as in similar cases, to abort the program ? I tend to think that
even raising a SIGSEGV manually would be better than silent undefined
behaviour.

  I can also confirm that strftime() fails to null-terminate its output in some
circumstances, most likely when the format string ends with a regular character
instead of a conversion specifier. I'll submit a patch as soon as I have time to
investigate more, unless you can fix it before I get to it.

-- 
  Laurent


[-- Attachment #2: leapsecs.patch --]
[-- Type: text/plain, Size: 4078 bytes --]

diff -rNU3 musl-old/src/time/__secs_to_tm.c musl/src/time/__secs_to_tm.c
--- musl-old/src/time/__secs_to_tm.c	2013-11-25 11:57:25.138608721 +0100
+++ musl/src/time/__secs_to_tm.c	2013-11-26 10:55:00.944118059 +0100
@@ -8,6 +8,21 @@
 #define DAYS_PER_100Y (365*100 + 24)
 #define DAYS_PER_4Y   (365*4   + 1)
 
+#ifndef NO_LEAPSECONDS
+static int leapsecs_sub(long long *t)
+{
+	unsigned int i = 0;
+	int hit = 0;
+	for (; i < __leapsecs_num; i++)
+		if (*t >= __leapsecs_table[i].trans) break;
+	if (i < __leapsecs_num) {
+		if (*t == __leapsecs_table[i].trans) hit = 1;
+		*t -= __leapsecs_table[i].corr;
+	}
+	return hit;
+}
+#endif
+
 int __secs_to_tm(long long t, struct tm *tm)
 {
 	long long days, secs;
@@ -21,6 +36,9 @@
 	if (t < INT_MIN * 31622400LL || t > INT_MAX * 31622400LL)
 		return -1;
 
+#ifndef NO_LEAPSECONDS
+	int hit = leapsecs_sub(&t);
+#endif
 	secs = t - LEAPOCH;
 	days = secs / 86400;
 	remsecs = secs % 86400;
@@ -76,6 +94,9 @@
 	tm->tm_hour = remsecs / 3600;
 	tm->tm_min = remsecs / 60 % 60;
 	tm->tm_sec = remsecs % 60;
+#ifndef NO_LEAPSECONDS
+	if (hit) tm->tm_sec++;
+#endif
 
 	return 0;
 }
diff -rNU3 musl-old/src/time/time_impl.h musl/src/time/time_impl.h
--- musl-old/src/time/time_impl.h	2013-11-25 11:57:25.138608721 +0100
+++ musl/src/time/time_impl.h	2013-11-26 10:48:40.712450191 +0100
@@ -7,3 +7,12 @@
 int __secs_to_tm(long long, struct tm *);
 void __secs_to_zone(long long, int, int *, long *, long *, const char **);
 const unsigned char *__map_file(const char *, size_t *);
+
+#ifndef NO_LEAPSECONDS
+struct lsinfo_s {
+  long long trans;
+  int corr;
+} ;
+extern struct lsinfo_s *__leapsecs_table;
+extern unsigned int __leapsecs_num;
+#endif
diff -rNU3 musl-old/src/time/__tm_to_secs.c musl/src/time/__tm_to_secs.c
--- musl-old/src/time/__tm_to_secs.c	2013-11-25 11:57:25.138608721 +0100
+++ musl/src/time/__tm_to_secs.c	2013-11-26 10:54:16.153921563 +0100
@@ -1,5 +1,16 @@
 #include "time_impl.h"
 
+#ifndef NO_LEAPSECONDS
+static void leapsecs_add(long long *t, int hit)
+{
+	unsigned int i = 0;
+	for (; i < __leapsecs_num; i++)
+		if (*t >= __leapsecs_table[i].trans) break;
+	if (i < __leapsecs_num)
+		*t += __leapsecs_table[i].corr + (hit && (*t == __leapsecs_table[i].trans));
+}
+#endif
+
 long long __tm_to_secs(const struct tm *tm)
 {
 	int is_leap;
@@ -20,5 +31,8 @@
 	t += 3600LL * tm->tm_hour;
 	t += 60LL * tm->tm_min;
 	t += tm->tm_sec;
+#ifndef NO_LEAPSECONDS
+	leapsecs_add(&t, tm->tm_sec==60);
+#endif
 	return t;
 }
diff -rNU3 musl-old/src/time/__tz.c musl/src/time/__tz.c
--- musl-old/src/time/__tz.c	2013-11-25 11:57:25.138608721 +0100
+++ musl/src/time/__tz.c	2013-11-26 12:41:31.142337204 +0100
@@ -118,6 +118,28 @@
 
 int __munmap(void *, size_t);
 
+#ifndef NO_LEAPSECONDS
+
+#define LEAPSECONDS_MAX 50  /* value in tzcode, probably too much */
+static struct lsinfo_s leapsecond_table[LEAPSECONDS_MAX<<1];
+struct lsinfo_s *__leapsecs_table = leapsecond_table;
+unsigned int __leapsecs_num = 0;
+
+static inline uint64_t zi_read64(const unsigned char *z)
+{
+	return ((uint64_t)zi_read32(z) << 32) + zi_read32(z+4);
+}
+
+static void parse_leapsecs(const unsigned char *z, unsigned int i, int lsize)
+{
+	__leapsecs_num = i;
+	for (; i; i--, z += lsize+4) {
+		__leapsecs_table[i-1].trans = (long long)(lsize == 8 ? zi_read64(z) : zi_read32(z));
+		__leapsecs_table[i-1].corr = zi_read32(z+lsize);
+	}
+}
+#endif
+
 static void do_tzset()
 {
 	char buf[NAME_MAX+25], *pathname=buf+24;
@@ -175,7 +197,7 @@
 	zi = map;
 	if (map) {
 		int scale = 2;
-		if (sizeof(time_t) > 4 && map[4]=='2') {
+		if (sizeof(time_t) > 4 && ((map[4]=='2') || (map[4]=='3'))) {
 			size_t skip = zi_dotprod(zi+20, VEC(1,1,8,5,6,1), 6);
 			trans = zi+skip+44+44;
 			scale++;
@@ -186,6 +208,9 @@
 		types = index + zi_read32(trans-12);
 		abbrevs = types + 6*zi_read32(trans-8);
 		abbrevs_end = abbrevs + zi_read32(trans-4);
+#ifndef NO_LEAPSECONDS
+		parse_leapsecs(abbrevs_end, zi_read32(trans-16), 1 << scale);
+#endif
 		if (zi[map_size-1] == '\n') {
 			for (s = (const char *)zi+map_size-2; *s!='\n'; s--);
 			s++;

Re: [PATCH] Add support for leap seconds in zoneinfo files

[Laurent Bercot]

>   Attached.

  And of course I reviewed it 3 times, and it's right after sending it than I
see an error. The definition of leapsecond_table should be
static struct lsinfo_s leapsecond_table[LEAPSECONDS_MAX];
(LEAPSECONDS_MAX elements instead of LEAPSECONDS_MAX<<1. Since that bug does
nothing but waste a few bytes, I didn't catch it when testing.)

-- 
  Laurent

Re: [PATCH] Add support for leap seconds in zoneinfo files

[Rich Felker]

On Tue, Nov 26, 2013 at 06:53:41PM +0000, Laurent Bercot wrote:
> 
>  Attached.
> 
>  I tried to make it as simple and musl-coding-style-compliant as possible.
>  One line adds support for TZif3 (it costs nothing once we have TZif2 and
> we can ignore the extensions).

That should be ok.

>  The other changes are ifdef-guarded and can be ignored with -DNO_LEAPSECONDS,
> for hardcore POSIX fans. ;) But posix/ rules don't have leap seconds anyway,
> only right/ ones do: if you don't like #ifdef forests, feel free to remove the
> guards.

That would probably be done for commit, but it's not important at this
time.

>  By the way, what's the musl policy on internal sanity checks ? In __tz.c,
> musl doesn't check anything after the magic numbers, so if the zoneinfo file
> is bad later on, musl can crash, or worse, silently return inconsistent or
> wrong results. I understand that it's impossible to escalate an error to the
> user when the entry point returns a void, but wouldn't it be safer, in this
> case as well as in similar cases, to abort the program ? I tend to think that
> even raising a SIGSEGV manually would be better than silent undefined
> behaviour.

The code is careful to restrict TZ to a system-wide timezone file if
running with elevated privileges. In other cases, providing a corrupt
or intentionally invalid file is user error, comparaible to providing
a corrupt or invalid .so file. I agree it would be "nice" to avoid
runaway incorrect behavior on invalid files, but I question whether
the cost is justifiable. Not only would it require a full zoneinfo
file validator; it would also require loading the file into memory,
rather than mmapping it, since with mmap the contents could (at least
in theory) change after validation is performed (sadly, Linux lacks
MAP_COPY).

By the way, this brings me to one part of the proposed patch that I'd
definitely like to see changed: the pre-parsing the leapseconds data
and storage in static memory. This both wastes static storage (800
bytes for 64-bit systems) and puts an arbitrary limit on the data size
where it's not needed. I'd really prefer that the leapsecond
processing happen direct from the mmapped space, just like the way
time zone transitions are handled. It would also be preferable to use
a binary search like what's used on the transition list instead of a
linear search; this change to binary search would probably more than
compensate for any performance loss from reading directly from the
mmapped data.

Other than that, I have not yet reviewed the patch in detail, but so
far it doesn't look too invasive.

>  I can also confirm that strftime() fails to null-terminate its output in some
> circumstances, most likely when the format string ends with a regular character
> instead of a conversion specifier. I'll submit a patch as soon as I have time to
> investigate more, unless you can fix it before I get to it.

I'll look into it and see if I can fix it first.

Rich

Re: [PATCH] Add support for leap seconds in zoneinfo files

[Rich Felker]

On Tue, Nov 26, 2013 at 06:32:12PM -0500, Rich Felker wrote:
> >  I can also confirm that strftime() fails to null-terminate its output in some
> > circumstances, most likely when the format string ends with a regular character
> > instead of a conversion specifier. I'll submit a patch as soon as I have time to
> > investigate more, unless you can fix it before I get to it.
> 
> I'll look into it and see if I can fix it first.

And I believe the issue has been fixed. See commit
http://git.musl-libc.org/cgit/musl/commit/?id=f63b8c8c455929f0f46cc017b4c675faeef901c4

Rich

Re: [PATCH] Add support for leap seconds in zoneinfo files

[Laurent Bercot]

> By the way, this brings me to one part of the proposed patch that I'd
> definitely like to see changed: the pre-parsing the leapseconds data
> and storage in static memory. This both wastes static storage (800
> bytes for 64-bit systems) and puts an arbitrary limit on the data size
> where it's not needed. I'd really prefer that the leapsecond
> processing happen direct from the mmapped space, just like the way
> time zone transitions are handled.

  Ok, but that will be more invasive. Time zone transitions can all be
performed in __secs_to_zone, but leap second data needs to be available
for __secs_to_tm and __tm_to_secs, since we only want to perform leap
second calculations when converting from/to broken-down time, not when
returning raw seconds: so interface changes will be necessary.


> It would also be preferable to use
> a binary search like what's used on the transition list instead of a
> linear search; this change to binary search would probably more than
> compensate for any performance loss from reading directly from the
> mmapped data.

  I don't think it's needed. The leap second table is searched in
descending order, and most calls to time functions are made with the
current time, or something close to it, so a linear search will stop
at the first item in most cases. And the rare worst case is only 25
iterations for now. I'll change to binary search if you insist, but
I really feel it would add complexity for a very tiny benefit in the
rare case and no performance increase at all in the common case.

-- 
  Laurent

Re: [PATCH] Add support for leap seconds in zoneinfo files

[Rich Felker]

On Wed, Nov 27, 2013 at 04:10:35AM +0000, Laurent Bercot wrote:
> 
> >By the way, this brings me to one part of the proposed patch that I'd
> >definitely like to see changed: the pre-parsing the leapseconds data
> >and storage in static memory. This both wastes static storage (800
> >bytes for 64-bit systems) and puts an arbitrary limit on the data size
> >where it's not needed. I'd really prefer that the leapsecond
> >processing happen direct from the mmapped space, just like the way
> >time zone transitions are handled.
> 
>  Ok, but that will be more invasive. Time zone transitions can all be
> performed in __secs_to_zone, but leap second data needs to be available
> for __secs_to_tm and __tm_to_secs, since we only want to perform leap
> second calculations when converting from/to broken-down time, not when
> returning raw seconds: so interface changes will be necessary.

__secs_to_tm and __tm_to_secs are not the right places for applying
leap seconds, because they affect gmtime, which is specified strictly
by POSIX to have a particular relationship with time_t...

> >It would also be preferable to use
> >a binary search like what's used on the transition list instead of a
> >linear search; this change to binary search would probably more than
> >compensate for any performance loss from reading directly from the
> >mmapped data.
> 
>  I don't think it's needed. The leap second table is searched in
> descending order, and most calls to time functions are made with the
> current time, or something close to it, so a linear search will stop
> at the first item in most cases. And the rare worst case is only 25
> iterations for now. I'll change to binary search if you insist, but
> I really feel it would add complexity for a very tiny benefit in the
> rare case and no performance increase at all in the common case.

I'm faily indifferent on it, but I don't think the binary search is
significantly more complicated.

Rich

Re: [PATCH] Add support for leap seconds in zoneinfo files

[Laurent Bercot]

> __secs_to_tm and __tm_to_secs are not the right places for applying
> leap seconds, because they affect gmtime, which is specified strictly
> by POSIX to have a particular relationship with time_t...

  I disagree that they are not the right place for it.

  * the point of using TAI-10 instead of UTC is to have a linear system
clock, so leap seconds must not be applied to clock_gettime() and
friends.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/time.html and
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_15
are vague enough ("approximate the number of seconds that have elapsed since
the Epoch") for TAI-10 to be a compliant setup here. The numerical relationship
between number of seconds and broken-down time is not respected if there are
leap seconds, but:

  * http://pubs.opengroup.org/onlinepubs/9699919799/functions/gmtime.html
makes it very clear that gmtime() should return UTC, and user applications
like "date" rely on it, so if there are leap seconds, they should definitely
be applied here;

  * POSIX followers will use posix/ timezones with 0 leap seconds anyway,
so the numerical relationship will be respected.

  To sum it up:
  - leap seconds break POSIX anyway, but should break as little as possible
  - leap second users only care about system clock time, not broken-down time
  - so gmtime() should always return UTC, this is relied on by userland so
it's more important than the exact relationship between tm and secs
  - so time() should ignore leap seconds but broken-down time should apply them
  - so the right place to apply them is in __secs_to_tm and __tm_to_secs, the
conversion routines
  - using a posix/ zone will make everything POSIX in any case.

-- 
Laurent

Re: [PATCH] Add support for leap seconds in zoneinfo files

[Szabolcs Nagy]

* Laurent Bercot <ska-dietlibc@skarnet.org> [2013-11-27 05:53:28 +0000]:
>  * the point of using TAI-10 instead of UTC is to have a linear system
> clock, so leap seconds must not be applied to clock_gettime() and
> friends.

TAI is non-linear for every moving object in the universe
it's wobbling and inaccurate like UT1 just on a smaller scale

the mistake was made in 1972 to broadcast leapsecond adjusted
atomic time for civil usage (UTC) instead of the earlier elastic
seconds (UT1) based on the rotation of the earth or plain SI
seconds (TAI,GPS,..) with deltaT information for display

there is an effort to redefine UTC though
http://www.ucolick.org/~sla/leapsecs/

>  To sum it up:
>  - leap seconds break POSIX anyway, but should break as little as possible
>  - leap second users only care about system clock time, not broken-down time
>  - so gmtime() should always return UTC, this is relied on by userland so
> it's more important than the exact relationship between tm and secs
>  - so time() should ignore leap seconds but broken-down time should apply them
>  - so the right place to apply them is in __secs_to_tm and __tm_to_secs, the
> conversion routines
>  - using a posix/ zone will make everything POSIX in any case.

i think with the leap second patch localtime is no longer correct:

 __secs_to_tm((long long)*t - tm->__tm_gmtoff, tm) < 0

the leap second will be added at the wrong time
(briefly making gmtime - localtime != gmtoff)

Re: [PATCH] Add support for leap seconds in zoneinfo files

[Laurent Bercot]

> i think with the leap second patch localtime is no longer correct:
>
>   __secs_to_tm((long long)*t - tm->__tm_gmtoff, tm) < 0

  Ah, good catch, the correction shoud also apply to the time given
to __secs_to_zone().
  I'll fix that in the next version of the patch.

-- 
  Laurent

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Laurent Bercot]

[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]


  OK, so actually, the patch was right: in a zoneinfo file containing leap
seconds, the transition times are given in TAI-10, not "UTC seconds", i.e.
times with leap seconds *added*.
(Yes, this is confusing, and the tzfile documentation does not make it
clear enough, but it's the only behaviour that makes sense.)
  So all the calculations involving times given in seconds, including
transition times, are done within the same referential. i.e. TAI-10,
and this is also valid for __tm_gmtoff.
  Only when converting from/to "seconds since the Epoch" to/from "struct tm"
must leap seconds be substracted/added.

  The new version of the patch, provided in attachment, reads leap seconds
directly from the mmapped zoneinfo file instead of storing them in a static
table, as requested by Rich; and it's still not invasive.
  The leap second table scan is still linear, because:
  * when substracting leap seconds, the scan is done from the future to the past.
Since the huge majority of calls involve the current time or a time close to it,
the scan usually stops at the first or second element.
  * when adding leap seconds, the scan has to be done from the past to the future
linearly anyway, to apply successive corrections while testing the correct TAI-10
transition time instead of comparing a UTC value to a TAI-10 value. The previous
version of the patch didn't get this right.

  I would love it if this version, modulo any bugs, could make it into 0.9.15.

-- 
  Laurent


[-- Attachment #2: leapsecs.patch --]
[-- Type: text/plain, Size: 3819 bytes --]

diff -rNU3 src-old/src/time/__secs_to_tm.c src/src/time/__secs_to_tm.c
--- src-old/src/time/__secs_to_tm.c	2013-12-04 15:50:13.000000000 +0100
+++ src/src/time/__secs_to_tm.c	2013-12-05 01:34:35.000000000 +0100
@@ -8,6 +8,23 @@
 #define DAYS_PER_100Y (365*100 + 24)
 #define DAYS_PER_4Y   (365*4   + 1)
 
+static int leapsecs_sub(long long *t)
+{
+	long long trans;
+	int corr;
+	unsigned int i = __leapsecs_num;
+	int hit = 0;
+	for (; i; i--) {
+		__leapsecs_read(i-1, &trans, &corr);
+		if (*t >= trans) break;
+	}
+	if (i) {
+		if (*t == trans) hit = 1;
+		*t -= corr;
+	}
+	return hit;
+}
+
 int __secs_to_tm(long long t, struct tm *tm)
 {
 	long long days, secs;
@@ -21,6 +38,7 @@
 	if (t < INT_MIN * 31622400LL || t > INT_MAX * 31622400LL)
 		return -1;
 
+	int hit = leapsecs_sub(&t);
 	secs = t - LEAPOCH;
 	days = secs / 86400;
 	remsecs = secs % 86400;
@@ -76,6 +94,7 @@
 	tm->tm_hour = remsecs / 3600;
 	tm->tm_min = remsecs / 60 % 60;
 	tm->tm_sec = remsecs % 60;
+	if (hit) tm->tm_sec++;
 
 	return 0;
 }
diff -rNU3 src-old/src/time/__tm_to_secs.c src/src/time/__tm_to_secs.c
--- src-old/src/time/__tm_to_secs.c	2013-12-04 15:50:13.000000000 +0100
+++ src/src/time/__tm_to_secs.c	2013-12-05 01:38:39.000000000 +0100
@@ -1,5 +1,21 @@
 #include "time_impl.h"
 
+static void leapsecs_add(long long *t, int hit)
+{
+	int oldcorr = 0;
+	unsigned int i = 0;
+	for (; i < __leapsecs_num; i++) {
+		long long trans;
+		int newcorr;
+		__leapsecs_read(i, &trans, &newcorr);
+		if (*t < trans) break;
+		if (!hit || (*t > trans)) {
+			*t += newcorr - oldcorr;
+		}
+		oldcorr = newcorr;
+	}
+}
+
 long long __tm_to_secs(const struct tm *tm)
 {
 	int is_leap;
@@ -20,5 +36,6 @@
 	t += 3600LL * tm->tm_hour;
 	t += 60LL * tm->tm_min;
 	t += tm->tm_sec;
+	leapsecs_add(&t, tm->tm_sec==60);
 	return t;
 }
diff -rNU3 src-old/src/time/__tz.c src/src/time/__tz.c
--- src-old/src/time/__tz.c	2013-12-04 15:50:13.000000000 +0100
+++ src/src/time/__tz.c	2013-12-05 01:42:19.000000000 +0100
@@ -17,6 +17,8 @@
 static char dst_name[TZNAME_MAX+1];
 const char __gmt[] = "GMT";
 
+unsigned int __leapsecs_num;
+
 static int dst_off;
 static int r0[5], r1[5];
 
@@ -105,6 +107,22 @@
 	return (unsigned)z[0]<<24 | z[1]<<16 | z[2]<<8 | z[3];
 }
 
+static uint64_t zi_read64(const unsigned char *z)
+{
+	return ((uint64_t)zi_read32(z) << 32) | (uint64_t)zi_read32(z+4);
+}
+
+void __leapsecs_read(unsigned int i, long long *tt, int *corr)
+{
+	if (trans > zi+44) {
+		*tt = (long long)zi_read64(abbrevs_end + 12*i);
+		*corr = (int)zi_read32(abbrevs_end + 12*i + 8);
+	} else {
+		*tt = (long long)zi_read32(abbrevs_end + 8*i);
+		*corr = (int)zi_read32(abbrevs_end + 8*i + 4);
+	}
+}
+
 static size_t zi_dotprod(const unsigned char *z, const unsigned char *v, size_t n)
 {
 	size_t y;
@@ -175,7 +193,7 @@
 	zi = map;
 	if (map) {
 		int scale = 2;
-		if (sizeof(time_t) > 4 && map[4]=='2') {
+		if (sizeof(time_t) > 4 && (map[4]=='2') || (map[4]=='3')) {
 			size_t skip = zi_dotprod(zi+20, VEC(1,1,8,5,6,1), 6);
 			trans = zi+skip+44+44;
 			scale++;
@@ -186,6 +204,7 @@
 		types = index + zi_read32(trans-12);
 		abbrevs = types + 6*zi_read32(trans-8);
 		abbrevs_end = abbrevs + zi_read32(trans-4);
+		__leapsecs_num = zi_read32(trans-16);
 		if (zi[map_size-1] == '\n') {
 			for (s = (const char *)zi+map_size-2; *s!='\n'; s--);
 			s++;
diff -rNU3 src-old/src/time/time_impl.h src/src/time/time_impl.h
--- src-old/src/time/time_impl.h	2013-12-04 15:50:13.000000000 +0100
+++ src/src/time/time_impl.h	2013-12-04 16:50:21.000000000 +0100
@@ -7,3 +7,5 @@
 int __secs_to_tm(long long, struct tm *);
 void __secs_to_zone(long long, int, int *, long *, long *, const char **);
 const unsigned char *__map_file(const char *, size_t *);
+extern unsigned int __leapsecs_num;
+void __leapsecs_read(unsigned int, long long *, int *);

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Szabolcs Nagy]

* Laurent Bercot <ska-dietlibc@skarnet.org> [2013-12-05 01:13:30 +0000]:
>  
> +static int leapsecs_sub(long long *t)
> +{
> +	long long trans;
> +	int corr;
> +	unsigned int i = __leapsecs_num;
> +	int hit = 0;
> +	for (; i; i--) {
> +		__leapsecs_read(i-1, &trans, &corr);
> +		if (*t >= trans) break;
> +	}
> +	if (i) {
> +		if (*t == trans) hit = 1;
> +		*t -= corr;
> +	}
> +	return hit;
> +}
>  
> +static void leapsecs_add(long long *t, int hit)
> +{
> +	int oldcorr = 0;
> +	unsigned int i = 0;
> +	for (; i < __leapsecs_num; i++) {
> +		long long trans;
> +		int newcorr;
> +		__leapsecs_read(i, &trans, &newcorr);
> +		if (*t < trans) break;
> +		if (!hit || (*t > trans)) {
> +			*t += newcorr - oldcorr;
> +		}
> +		oldcorr = newcorr;
> +	}
> +}
> +

i assume

	hit=leapsecs_sub(&t);
	if(hit) t++;
	leapsecs_add(&t,hit);

should give back the original t,
then the add part can go backwards on the leapsec list instead of forward:

static void leapsecs_add(long long *t, int hit)
{
	long long trans;
	int corr;
	unsigned int i = __leapsecs_num;
	for (; i; i--) {
		__leapsecs_read(i-1, &trans, &corr);
		if (*t+corr > trans) {
			*t += corr;
			// only consider hit at a leapsecond
			if (hit && *t == trans+1) *t--;
			break;
		}
	}
}

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Laurent Bercot]

> i assume
>
> 	hit=leapsecs_sub(&t);
> 	if(hit) t++;
> 	leapsecs_add(&t,hit);

  This is true if and only if all leap seconds are 1, which is the case for now
and in the foreseeable future, but my version supports even the improbable
case where different corrections are introduced.
  But you are right, it is possible to go backwards too:

static void leapsecs_add(long long *t, int hit)
{
	long long trans;
	int corr;
	unsigned int i = __leapsecs_num;
	for (; i; i--) {
		__leapsecs_read(i-1, &trans, &corr);
		if (*t + corr >= trans) break ;
	}
	if (i) 	{
		*t += corr;
		if (hit && (t == trans)) {
			int oldcorr = 0;
			if (i > 1)
				__leapsecs_read(i-2, &trans, &oldcorr);
			*t += oldcorr - corr;
		}
	}
}

-- 
  Laurent

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Szabolcs Nagy]

* Laurent Bercot <ska-dietlibc@skarnet.org> [2013-12-05 08:58:50 +0000]:
>  This is true if and only if all leap seconds are 1, which is the case for now
> and in the foreseeable future, but my version supports even the improbable
> case where different corrections are introduced.

"The current rules for leap seconds in UTC are specified by ITU-R TF.460.
They state that a leap second may be added (or subtracted) at the end of
any month, but that first preference is given to June and December, and
second preference is given to March and September."

this breaks down after <2000 years when there will be 12 leapseconds/year,
allowing 1 leapsecond/day will work for >40000 years

the UTC definition does not allow double leap seconds so posix and
iso c specify the [0,60] range for tm_sec (ie >1 leap seconds cannot
be supported, <-1 leap seconds could be supported, but since the
earth slows down that wont happen)

systems will not break because a big leap is introduced 10k years from
now, but because there is a day with 86401 seconds about every 2 years

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Rich Felker]

On Thu, Dec 05, 2013 at 01:13:30AM +0000, Laurent Bercot wrote:
>  The new version of the patch, provided in attachment, reads leap seconds
> directly from the mmapped zoneinfo file instead of storing them in a static
> table, as requested by Rich; and it's still not invasive.
>  The leap second table scan is still linear, because:
>  * when substracting leap seconds, the scan is done from the future to the past.
> Since the huge majority of calls involve the current time or a time close to it,
> the scan usually stops at the first or second element.
>  * when adding leap seconds, the scan has to be done from the past to the future
> linearly anyway, to apply successive corrections while testing the correct TAI-10
> transition time instead of comparing a UTC value to a TAI-10 value. The previous
> version of the patch didn't get this right.
> 
>  I would love it if this version, modulo any bugs, could make it into 0.9.15.

I don't want to disappoint you but I don't think that's feasible.
There's still a lot to be considered about this patch, and it _is_
invasive. The biggest invasive changes are the interface contract
violations for gmtime; after the patch:

- gmtime depends on the TZ variable

- gmtime behaves unpredictably depending on whether a function that
  loads the timezone has or has not been called prior to calling
  gmtime.

- there's no synchronization in gmtime's (or gmtime_r's) access to the
  leap seconds data so the latter isn't even thread-safe.

There may still be other things too I'm not aware of, but the above
are what I noticed immediately.

What I can tell you is that this code (the time internals) is not
something I expect to make any changes to between now and some time
after 1.0, so for your own usage or usage by anybody else who wants
it, your patch should continue to apply successfully. And I don't mind
further discussion of how to improve it in the mean time, but in the
immediate future my focus on musl will be getting 0.9.15 and 1.0.0
releases ready according to the existing plan, without big invasive
changes.

Rich

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Laurent Bercot]

  Eh, that's fine. I have workarounds for libcs that don't support
leap seconds. As long as that support is going to make it into musl
*eventually*, I'm not disappointed. So, let's discuss.


 > - gmtime depends on the TZ variable

  Yes.
  Functions that display human time need to derive it from the system
time, so they need to know what the system time is. POSIX sidesteps
the problem by saying "the system time will be aligned with some
human time (UTC)"; but if you do not want the system time to be
aligned with UTC, then you have to make those functions aware of what
the system time is.
  gmtime() cannot be the same function when your system time is UTC
and when it is TAI-10: it needs to be given that information.
Since zoneinfo files include that very information - the leap second 
table - then I figured it made sense to use them: the timezone tells 
gmtime() how to interpret the system time. Timezones are used not
only as a UTC <--> local time information provider, but also as a
UTC <--> system clock time information provider.
  Note that TZ values pointing to POSIX timezone definitions will never
change gmtime()'s behaviour.
  I am not fundamentally opposed to conveying the information in
another way, as long as it can be done consistently.


 > - gmtime behaves unpredictably depending on whether a function that
 >    loads the timezone has or has not been called prior to calling
 >    gmtime.

  This is indeed a bug - an oversight on my part, sorry.
  Would calling do_tzset() at the beginning of gmtime_r() fix it ? Would
it be prohibitively expensive, and if so, what would be a better
solution ?


 > - there's no synchronization in gmtime's (or gmtime_r's) access to the
 >    leap seconds data so the latter isn't even thread-safe.

  OK, if changing timezones during the lifetime of a process is a valid
use case, then things must be protected. There needs to be a read-only
lock around __leapsecs_add() and __leapsecs_sub(), and a read-write lock
around the setting of __leapsecs_num and abbrevs_end. I couldn't find
such a thing as a shared lock system in musl, though. Should I go for
LOCK and UNLOCK, even though it would uselessly prevent concurrent
reads, or is there a better way ?


 > What I can tell you is that this code (the time internals) is not
 > something I expect to make any changes to between now and some time
 > after 1.0, so for your own usage or usage by anybody else who wants
 > it, your patch should continue to apply successfully.

  I intend to maintain it until it gets integrated.


 > And I don't mind
 > further discussion of how to improve it in the mean time, but in the
 > immediate future my focus on musl will be getting 0.9.15 and 1.0.0
 > releases ready according to the existing plan, without big invasive
 > changes.

  Please consider integrating it for 1.0.0 if it meets all your
criteria by then.

-- 
  Laurent

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Rich Felker]

On Thu, Dec 05, 2013 at 04:31:38PM +0000, Laurent Bercot wrote:
> > - gmtime behaves unpredictably depending on whether a function that
> >    loads the timezone has or has not been called prior to calling
> >    gmtime.
> 
>  This is indeed a bug - an oversight on my part, sorry.
>  Would calling do_tzset() at the beginning of gmtime_r() fix it ? Would
> it be prohibitively expensive, and if so, what would be a better
> solution ?

The problem is that gmtime is not permitted to do so. tzset has side
effects like making the globals timezone and daylight change, and
certain time functions are specified to behave as if they call it,
while others are not (and therefore can't). The only way I see around
this is fairly invasive: possibly keeping around two timezones. Or
(almost equivalent) keeping timezone and leapseconds profiles
completely separate.

> > - there's no synchronization in gmtime's (or gmtime_r's) access to the
> >    leap seconds data so the latter isn't even thread-safe.
> 
>  OK, if changing timezones during the lifetime of a process is a valid
> use case, then things must be protected. There needs to be a read-only
> lock around __leapsecs_add() and __leapsecs_sub(), and a read-write lock
> around the setting of __leapsecs_num and abbrevs_end. I couldn't find
> such a thing as a shared lock system in musl, though. Should I go for
> LOCK and UNLOCK, even though it would uselessly prevent concurrent
> reads, or is there a better way ?

That's a good question. __mmap has such a minimal rwlock mechanism,
but it's currently not sharable (it's single-instance). However
presumably each use would have to check for change in the state
(change in $TZ) and possibly modify the data, so I'm not even clear
that rwlock would be appropriate.

What's more troubling is that the locks may need to be recursive (and
thus MUCH more expensive) since some functions might depend internally
(IIRC they do) on being able to make multiple calls to tm/time_t
conversion functions with consistent behavior, in which case they'd
have to hold the lock across multiple calls which might also use
internal locking.

Note that the problematic aspects that are coming up are most
problematic because they're affecting not just users who want
leapseconds, but all users. If leapseconds affect gmtime and
leapseconds depend on TZ, all the sudden these issues apply to all
calls.

Rich

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Laurent Bercot]

> The problem is that gmtime is not permitted to do so.  tzset has side
> effects like making the globals timezone and daylight change, and
> certain time functions are specified to behave as if they call it,
> while others are not (and therefore can't).

  I don't see anything in POSIX that forbids gmtime() to call tzset(),
but if you're referring to the case where a user calls gmtime() and
expects the "daylight" variable to remain unchanged, then yes, I agree
that it is a problem.


> The only way I see around
> this is fairly invasive: possibly keeping around two timezones. Or
> (almost equivalent) keeping timezone and leapseconds profiles
> completely separate.

  I'm all for keeping timezone information (which is about conversions
between UTC and local time) separate from leap second information (which
is about conversions between system time and UTC).
The tzdata package actually provides the leap second table as a separate
file, and it was pretty bad design to include it into the zoneinfo binary
files in the end.
  I have two suggestions:
     1. Keep using the leap second information provided in the timezone
files, but have a __leapsecs_init() function used by tzset, gmtime and
friends, that only initializes __leapsecs_num and abbrevs_end, without
other visible side effects. It requires mapping the file, so adding a
flag to know whether tzset() has been called or not. It still makes gmtime
depend on TZ.
  or 2. Compile a separate leap second information binary file along with
the libc, and have a system-wide switch telling musl to use the leap second
table or not. (And default to POSIX when the file does not exist, of
course.) This is the approach I'm using with skalibs, and it's working for
me, but the requirements for a libc may be different.


> [locking issues]
> Note that the problematic aspects that are coming up are most
> problematic because they're affecting not just users who want
> leapseconds, but all users. If leapseconds affect gmtime and
> leapseconds depend on TZ, all the sudden these issues apply to all
> calls.

  This goes in favour of the "keep the leap second table in a separate
file" design. We could drop the need for locking entirely, since a
file can be atomically replaced, and new leap second announcements are
rare enough that the policy could simply be to restart long-lived
processes (to get the new mapping) when the file changes.
  Strictly POSIX systems wouldn't be impacted at all anyway.

-- 
  Laurent

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Rich Felker]

On Fri, Dec 06, 2013 at 12:36:29AM +0000, Laurent Bercot wrote:
> 
> >The problem is that gmtime is not permitted to do so.  tzset has side
> >effects like making the globals timezone and daylight change, and
> >certain time functions are specified to behave as if they call it,
> >while others are not (and therefore can't).
> 
>  I don't see anything in POSIX that forbids gmtime() to call tzset(),

There's also nothing in POSIX that says calling memset can't close all
open files. The reason it can't happen is the lack of any text
allowing it to happen. Functions in the standard cannot have any side
effects visible to a conforming application aside from those they're
specified or otherwise explicitly allowed to have.

A better example might be fgets. Some people have argued that by
giving fgets a pointer to a buffer, you're giving it permission to
write to any part of the buffer. This is not the case. fgets is
specified to behave as if by repeated calls to fgetc, and therefore it
cannot write to any position in the buffer except for the positions it
would write as if implemented by repeat calls to fgetc and sequential
storage.

> but if you're referring to the case where a user calls gmtime() and
> expects the "daylight" variable to remain unchanged, then yes, I agree
> that it is a problem.

Exactly. I'm referring to visible side effects.

> >The only way I see around
> >this is fairly invasive: possibly keeping around two timezones. Or
> >(almost equivalent) keeping timezone and leapseconds profiles
> >completely separate.
> 
>  I'm all for keeping timezone information (which is about conversions
> between UTC and local time) separate from leap second information (which
> is about conversions between system time and UTC).
> The tzdata package actually provides the leap second table as a separate
> file, and it was pretty bad design to include it into the zoneinfo binary
> files in the end.
>  I have two suggestions:
>     1. Keep using the leap second information provided in the timezone
> files, but have a __leapsecs_init() function used by tzset, gmtime and
> friends, that only initializes __leapsecs_num and abbrevs_end, without
> other visible side effects. It requires mapping the file, so adding a
> flag to know whether tzset() has been called or not. It still makes gmtime
> depend on TZ.
>  or 2. Compile a separate leap second information binary file along with
> the libc, and have a system-wide switch telling musl to use the leap second
> table or not. (And default to POSIX when the file does not exist, of
> course.) This is the approach I'm using with skalibs, and it's working for
> me, but the requirements for a libc may be different.

These approaches sound closer to something that might be acceptable.

Regarding the unfortunate coupling of timezone and leapsecond data
into a single file, do the zoneinfo files with leapseconds have their
timezone transition times stored in POSIX time or TAI?

> >[locking issues]
> >Note that the problematic aspects that are coming up are most
> >problematic because they're affecting not just users who want
> >leapseconds, but all users. If leapseconds affect gmtime and
> >leapseconds depend on TZ, all the sudden these issues apply to all
> >calls.
> 
>  This goes in favour of the "keep the leap second table in a separate
> file" design. We could drop the need for locking entirely, since a
> file can be atomically replaced, and new leap second announcements are
> rare enough that the policy could simply be to restart long-lived
> processes (to get the new mapping) when the file changes.

It seems really inconsistent to me that someone who's pedantic enough
to care about leap seconds wouldn't care about having to "reboot" (I
use this term loosely) their servers/daemons when a new one is
announced. If you're ok with that discontinuity what's wrong with just
losing the leap second?

>  Strictly POSIX systems wouldn't be impacted at all anyway.

At the very least they would have some overhead determining that
leapseconds aren't in use.

Rich

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Laurent Bercot]

> Regarding the unfortunate coupling of timezone and leapsecond data
> into a single file, do the zoneinfo files with leapseconds have their
> timezone transition times stored in POSIX time or TAI?

  TAI-10 (the 10 seconds offset is so it's the same as POSIX time
before 1972). As I said, it's the only behaviour that makes sense:
we're counting a number of seconds and we want it to be unambiguous.
POSIX time would create an ambiguity for the very moment when a leap
second happens, which defeats the purpose.


> It seems really inconsistent to me that someone who's pedantic enough
> to care about leap seconds wouldn't care about having to "reboot" (I
> use this term loosely) their servers/daemons when a new one is
> announced. If you're ok with that discontinuity what's wrong with just
> losing the leap second?

  Leap seconds are announced in advance; it's easy to plan a "reboot"
over a 6-month (or larger) period before the actual leap second happens.
  On the other hand, no planning will prevent the kernel from crashing,
make from getting confused, CLOCK_REALTIME-based event loops from failing
when the system clock hiccups under their feet.

  Locking and an advanced updating system to the leap second table could
be designed, so that long-lived processes don't even have to restart when
a new leap second is announced. But frankly, it just doesn't seem worth the
trouble. Restarting a process is easy; people who need high availability
usually have several instances of the service and can restart them one by
one.


> At the very least they would have some overhead determining that
> leapseconds aren't in use.

  Indeed, one test at executable start. :)
  Do you have any idea what form this test would take, if we're opting
away from TZ ?

-- 
  Laurent

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Szabolcs Nagy]

* Laurent Bercot <ska-dietlibc@skarnet.org> [2013-12-06 01:15:52 +0000]:
> >At the very least they would have some overhead determining that
> >leapseconds aren't in use.
> 
>  Indeed, one test at executable start. :)
>  Do you have any idea what form this test would take, if we're opting
> away from TZ ?

- musl should not have configuration options that change
  the behaviour of an api (otherwise maintaining the code
  becomes much harder) so making leapseconds a libc build
  time config option does not work

- hardcoding leapsecond tables into libc does not work either
  requires recompilations and restarts

- if the clock source is a kernel level setting then the
  information should come from the kernel (eg in the aux
  vector or /proc)

- if the clock source setting is an external setting (eg the
  configured time server uses GPS or TAI time scale) then the
  admin should provide the information through the filesystem,
  but an update is still an operational hazard, and we need
  to invent a file format and provide tools to get it from the
  distributed tzdata files

- during the life-time of a process adding leapseconds may
  cause problems so the data should be cached at startup or
  on demand at the first gmtime call, but then there might be
  problems across processes if they don't have the same cached
  leapsecond list

(the 86401 second day can cause problems no matter how we do
this: communicating unix time across systems with different
time scale or inserting future dates into databases based on
unix time when the leap seconds are not yet known, etc)

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Laurent Bercot]

> - musl should not have configuration options that change
>    the behaviour of an api (otherwise maintaining the code
>    becomes much harder) so making leapseconds a libc build
>    time config option does not work

  Also, that would break musl's policy of "compile once, run
everywhere". Leap second usage should be a system-wide setting,
not a binary-wide setting.


> - hardcoding leapsecond tables into libc does not work either
>    requires recompilations and restarts

  Of course. Leap second tables should be read either from the
timezone files, as glibc does, or from a separate file.


> - if the clock source is a kernel level setting then the
>    information should come from the kernel (eg in the aux
>    vector or /proc)
> - if the clock source setting is an external setting (eg the
>    configured time server uses GPS or TAI time scale) then the
>    admin should provide the information through the filesystem,

  musl shouldn't try to autodetect settings from the kernel. The
admin should be the authoritative source of information.


>    but an update is still an operational hazard, and we need
>    to invent a file format and provide tools to get it from the
>    distributed tzdata files

  That's the easy part. I'm already doing that for skalibs. I'm
willing to provide the format and the tools.


> - during the life-time of a process adding leapseconds may
>    cause problems so the data should be cached at startup or
>    on demand at the first gmtime call, but then there might be
>    problems across processes if they don't have the same cached
>    leapsecond list

  Again, leap second announcements are a rare event and there's
plenty of time for admins to restart their processes. I don't
think that's something musl should care about.

-- 
  Laurent

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Raphael Cohn]

[-- Attachment #1: Type: text/plain, Size: 3433 bytes --]

I'm not sure I've completely followed all the ramifications here. Why
should one have to restart processes on 1,000s of nodes for something like
this? 5 9s contracts are hard enough to satisfy at the best of times,
without adding another 30 - 120 seconds of restart impact, degraded
performance and failover for large jobs. In the sorts of sites we operate
in, there might be just one admin for the entire estate, and they're busy
enough, and probably laden down with more special knowledge than they can
recall as it is, to have to deal with something like this as well...

From this point of view, a system update of packages and data files should
be accommodated automatically. Isn't that what a file watch is for?
Actually, I'd argue the same for any change to any file of configuration
data used by a library. There's no way an userspace app is going to know
with certainty what files its underlying linked libs are using or how.

That said, I'm all for getting this right - but I'll leave it to others
what that means, with one caveat. Date and time match has been got wrong in
every system (eg a swap pricing engine, a national telco ordering system)
and programming language I've used where it actually mattered... Java are
on their third cut at it, for starters... Personally, I think apps should
just use a monotonic source of seconds from an epoch, and use a
well-developed third party lib dedicated to the problem if they need date
math (eg Joda time in Java).


On 6 December 2013 10:48, Laurent Bercot <ska-dietlibc@skarnet.org> wrote:

>
>  - musl should not have configuration options that change
>>    the behaviour of an api (otherwise maintaining the code
>>    becomes much harder) so making leapseconds a libc build
>>    time config option does not work
>>
>
>  Also, that would break musl's policy of "compile once, run
> everywhere". Leap second usage should be a system-wide setting,
> not a binary-wide setting.
>
>
>
>  - hardcoding leapsecond tables into libc does not work either
>>    requires recompilations and restarts
>>
>
>  Of course. Leap second tables should be read either from the
> timezone files, as glibc does, or from a separate file.
>
>
>
>  - if the clock source is a kernel level setting then the
>>    information should come from the kernel (eg in the aux
>>    vector or /proc)
>> - if the clock source setting is an external setting (eg the
>>    configured time server uses GPS or TAI time scale) then the
>>    admin should provide the information through the filesystem,
>>
>
>  musl shouldn't try to autodetect settings from the kernel. The
> admin should be the authoritative source of information.
>
>
>
>     but an update is still an operational hazard, and we need
>>    to invent a file format and provide tools to get it from the
>>    distributed tzdata files
>>
>
>  That's the easy part. I'm already doing that for skalibs. I'm
> willing to provide the format and the tools.
>
>
>
>  - during the life-time of a process adding leapseconds may
>>    cause problems so the data should be cached at startup or
>>    on demand at the first gmtime call, but then there might be
>>    problems across processes if they don't have the same cached
>>    leapsecond list
>>
>
>  Again, leap second announcements are a rare event and there's
> plenty of time for admins to restart their processes. I don't
> think that's something musl should care about.
>
> --
>  Laurent
>
>

[-- Attachment #2: Type: text/html, Size: 4885 bytes --]

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Laurent Bercot]

On 06/12/2013 11:38, Raphael Cohn wrote:

> I'm not sure I've completely followed all the ramifications here.
>  Why should one have to restart processes on 1,000s of nodes
>  for something like this? 5 9s contracts are hard enough to satisfy
>  at the best of times, without adding another 30 - 120 seconds of
>  restart impact, degraded performance and failover for large jobs.
>  In the sorts of sites we operate in, there might be just one admin
>  for the entire estate, and they're busy enough, and probably laden
>  down with more special knowledge than they can recall as it is, to
>  have to deal with something like this as well...

  The recent rate of leap seconds announcements has been about one
every three years. Firmware, and even hardware, evolves faster than that.
In three years, you probably have had to update your production image at
least once. Including new leap second tables into it would basically have
been free.
  And even if you have to do a restart *specifically* for this thing,
if you are dealing with thousands of nodes and need to provide five nines,
then you have good enough automation to reboot your machines one after
the other by snapping your fingers and without making a dent in the
quality of your service.


>  From this point of view, a system update of packages and data files
>  should be accommodated automatically. Isn't that what a file watch is for?
>  Actually, I'd argue the same for any change to any file of configuration
>  data used by a library. There's no way an userspace app is going to know
>  with certainty what files its underlying linked libs are using or how.

  It's a question of balancing complexity. File watches and such can be
used, but:
  - it's hard, it implies more code in the libc, and probably more run-time
resource usage.
  - it's a leaky abstraction: user applications have to avoid caching state,
because that state could change under their feet - which is precisely what
I'm trying to avoid here.
  - this point depends on what configuration data we're talking about, but
as far as leap seconds are concerned, that data is not very dynamic. It's
almost static. Do your daemons really have three years of uptime ? Random
hardware crashes will be more of an annoyance than this.

  Restarting a process is cheap and easy, should be a standard tool in your
toolbox, and your workflow should make use of it when it's needed. You
don't get high availability by forbidding your processes to die: you get
high availability by making sure it's not a problem when they die.


> Date and time match has been got wrong in every system
> (...)
> Personally, I think apps should just use a monotonic source of seconds
>  from an epoch, and use a well-developed third party lib dedicated to
>  the problem if they need date math (eg Joda time in Java).

  I absolutely agree with you on the first part. I disagree on the second
part. Dealing with time shouldn't be a burden on the application - devs
have other things to think about, and experience shows that most of them
won't care, they'll just use the primitives provided by the system. So,
the system should do the right thing, i.e. provide something that works
no matter how applications are using it. Here, it means providing a
linear CLOCK_REALTIME, because people use it as if it were CLOCK_MONOTONIC.

-- 
  Laurent

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Rich Felker]

On Fri, Dec 06, 2013 at 11:29:33PM +0000, Laurent Bercot wrote:
> On 06/12/2013 11:38, Raphael Cohn wrote:
> 
> >I'm not sure I've completely followed all the ramifications here.
> > Why should one have to restart processes on 1,000s of nodes
> > for something like this? 5 9s contracts are hard enough to satisfy
> > at the best of times, without adding another 30 - 120 seconds of
> > restart impact, degraded performance and failover for large jobs.
> > In the sorts of sites we operate in, there might be just one admin
> > for the entire estate, and they're busy enough, and probably laden
> > down with more special knowledge than they can recall as it is, to
> > have to deal with something like this as well...
> 
>  The recent rate of leap seconds announcements has been about one
> every three years. Firmware, and even hardware, evolves faster than that.
> In three years, you probably have had to update your production image at
> least once. Including new leap second tables into it would basically have
> been free.
>  And even if you have to do a restart *specifically* for this thing,
> if you are dealing with thousands of nodes and need to provide five nines,
> then you have good enough automation to reboot your machines one after
> the other by snapping your fingers and without making a dent in the
> quality of your service.

What I'm missing is the benefit. The only benefit I can see to using
TAI-10 is that you're synchronizing with your chosen clock source over
years/decades with no discontinuity. If you're willing to accept a
discontinuity every few years, why not just use plain POSIX time and
apply the new difference between TAI-10 and POSIX time when rebooting?

>  Restarting a process is cheap and easy, should be a standard tool in your
> toolbox, and your workflow should make use of it when it's needed. You
> don't get high availability by forbidding your processes to die: you get
> high availability by making sure it's not a problem when they die.

I think this is an area where you'll find you're coming from a
fundamentally different philosophy than most of the musl folks. The
way I interpret your position is that using TAI-10 is only suitable
for managed desktop/server systems, not any other kind of use where
there's no system operator who's aware of leap second announcements
and capable of doing manual upgrasdes. (And having system scripts kill
and restart processes behind your back to do this for you is even
worse; that's akin to the way Windows automatically reboots behind
your back, and users hate it.)

> >Date and time match has been got wrong in every system
> >(...)
> >Personally, I think apps should just use a monotonic source of seconds
> > from an epoch, and use a well-developed third party lib dedicated to
> > the problem if they need date math (eg Joda time in Java).
> 
>  I absolutely agree with you on the first part. I disagree on the second
> part. Dealing with time shouldn't be a burden on the application - devs
> have other things to think about, and experience shows that most of them
> won't care, they'll just use the primitives provided by the system. So,
> the system should do the right thing, i.e. provide something that works
> no matter how applications are using it. Here, it means providing a
> linear CLOCK_REALTIME, because people use it as if it were CLOCK_MONOTONIC.

If that's your only goal, it's easily achieved without storing TAI-10
in the CLOCK_REALTIME clock, assuming by "linear" you just mean
"monotonic, continuous, and accurate within the margin of measurement
error". Discontinuous jumps and non-monotonicity in CLOCK_REALTIME
were a monstrosity invented by the NTPD folks in their implementation
for mapping TAI onto POSIX time, not any fundamental requirement.

Rich

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Szabolcs Nagy]

* Rich Felker <dalias@aerifal.cx> [2013-12-06 21:31:14 -0500]:
> On Fri, Dec 06, 2013 at 11:29:33PM +0000, Laurent Bercot wrote:
> > On 06/12/2013 11:38, Raphael Cohn wrote:
> > >Date and time match has been got wrong in every system
> > >(...)
> > >Personally, I think apps should just use a monotonic source of seconds
> > > from an epoch, and use a well-developed third party lib dedicated to
> > > the problem if they need date math (eg Joda time in Java).
> > 
> >  I absolutely agree with you on the first part. I disagree on the second
> > part. Dealing with time shouldn't be a burden on the application - devs
> > have other things to think about, and experience shows that most of them
> > won't care, they'll just use the primitives provided by the system. So,
> > the system should do the right thing, i.e. provide something that works
> > no matter how applications are using it. Here, it means providing a
> > linear CLOCK_REALTIME, because people use it as if it were CLOCK_MONOTONIC.
> 
> If that's your only goal, it's easily achieved without storing TAI-10
> in the CLOCK_REALTIME clock, assuming by "linear" you just mean
> "monotonic, continuous, and accurate within the margin of measurement
> error". Discontinuous jumps and non-monotonicity in CLOCK_REALTIME
> were a monstrosity invented by the NTPD folks in their implementation
> for mapping TAI onto POSIX time, not any fundamental requirement.

it's worse than that, the proposed solution does put a burden on userspace
(unix time can no longer be used to calculate or represent dates)
while with the standard 1 day == 86400 s apps and admins do not even need
to know about leapseconds (only the time daemon and kernel)

about linearity:

if you only smooth out a leapsecond in the month it was inserted, then a
unix second would be about 30*86400/(30*86400+1)-1 = -0.386 ppm (= 386ns)
shorter than a SI second in that month, by comparision (according to the
ntp clock quality faq) the oscillator used by most hw usually have more
than 1 ppm frequency error and it can change 1 ppm/C when heated up if no
temperature correction is done, so apps already have to deal with larger
errors and the smoothing logic is in the kernel/ntpd they just don't apply
it to leapseconds

this will work for more than 1000 years (earth rotation drift is <12s/year)
and somewhere during that time leapseconds will be abolished and it stops
being a problem..

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Laurent Bercot]

> What I'm missing is the benefit. The only benefit I can see to using
> TAI-10 is that you're synchronizing with your chosen clock source over
> years/decades with no discontinuity. If you're willing to accept a
> discontinuity every few years, why not just use plain POSIX time and
> apply the new difference between TAI-10 and POSIX time when rebooting?

  You don't choose when a leap second happens. To provide accurate time at
every instant your machine is up, you would have to schedule your
downtime right around a leap second. That's obviously not possible. My
suggestion allows you to choose and plan your discontinuity, most
probably merging it with some other discontinuity that will happen
anyway, while providing accurate time at every moment.


> I think this is an area where you'll find you're coming from a
> fundamentally different philosophy than most of the musl folks. The
> way I interpret your position is that using TAI-10 is only suitable
> for managed desktop/server systems, not any other kind of use where
> there's no system operator who's aware of leap second announcements
> and capable of doing manual upgrasdes. (And having system scripts kill
> and restart processes behind your back to do this for you is even
> worse; that's akin to the way Windows automatically reboots behind
> your back, and users hate it.)

  - Users hate it when Windows reboots behind their backs because it
obnoxiously gets in their way and disrupts their workflow. It's an obvious
loss of quality of service. Restarting a Unix process has nothing to do
with that. Even rebooting a whole Unix machine has nothing to do with that
if it's correctly planned.
  - Even embedded systems have firmware upgrades, and most embedded devices
automatically download the new firmware and switch to it the next time the
user powercycles the machine. I challenge the belief that a system without
any kind of operator is going to have a 3 years uptime. (And if it does,
it probably doesn't need to measure Earth time accurately.)
  - If there's such a demand for it, I can provide an implementation where you
don't even have to restart a process when the leap second table changes. It
will just be expensive at run-time (perform some kind of test whenever the
table is accessed, or simply open/read/close the file for every access). I
will ask for a musl compilation option to disable it, though, because I
believe it's useless and a waste of resources.


> If that's your only goal, it's easily achieved without storing TAI-10
> in the CLOCK_REALTIME clock, assuming by "linear" you just mean
> "monotonic, continuous, and accurate within the margin of measurement
> error".

  It probably becomes a matter of religion at this point. What is a second ?
is it Evil if a second isn't always the same duration ? Is it Evil to pretend
that some operation lasted 5 seconds when it actually was a bit more by the
usual definition of a second ?
  Until kernels perform leap smearing or provide CLOCK_UTS, and we manage to
coerce computers into using our imperfect, changing, human idea of time without
choking (my religion considers this as torture!), though, TAI-10 is the only
way to achieve this goal in practice.


> Discontinuous jumps and non-monotonicity in CLOCK_REALTIME
> were a monstrosity invented by the NTPD folks in their implementation
> for mapping TAI onto POSIX time, not any fundamental requirement.

  I agree that ntpd is a prime example of bad design in about every way, but
I blame POSIX more for forcing computers to align system time with human time
in the first place. :)

-- 
  Laurent

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Rich Felker]

On Fri, Dec 06, 2013 at 01:15:52AM +0000, Laurent Bercot wrote:
> 
> >Regarding the unfortunate coupling of timezone and leapsecond data
> >into a single file, do the zoneinfo files with leapseconds have their
> >timezone transition times stored in POSIX time or TAI?
> 
>  TAI-10 (the 10 seconds offset is so it's the same as POSIX time
> before 1972). As I said, it's the only behaviour that makes sense:
> we're counting a number of seconds and we want it to be unambiguous.
> POSIX time would create an ambiguity for the very moment when a leap
> second happens, which defeats the purpose.

The reason I asked is that it means, if you're using TAI-10, you have
to have matching zoneinfo files for the daylight time transitions to
work correctly. This really complicates the matter of having POSIX/TAI
and TZ be separate...

> >At the very least they would have some overhead determining that
> >leapseconds aren't in use.
> 
>  Indeed, one test at executable start. :)

At first use. Definitely not at executable start.

>  Do you have any idea what form this test would take, if we're opting
> away from TZ ?

No, I don't know.

Rich

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Laurent Bercot]

> The reason I asked is that it means, if you're using TAI-10, you have
> to have matching zoneinfo files for the daylight time transitions to
> work correctly. This really complicates the matter of having POSIX/TAI
> and TZ be separate...

  Yes, people using TAI-10 should also be using right/ timezones instead of
posix/ ones. That's why TZ as a test is appealing: it ensures consistency.
But at that level it's entirely the admin's responsibility to have
consistent configuration: leap-second-conscious admins won't mind doing
some extra work to have a setting that pleases them.


> At first use. Definitely not at executable start.

  Of course, that's what I meant, my bad.

-- 
  Laurent

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Rich Felker]

On Fri, Dec 06, 2013 at 10:37:56AM +0000, Laurent Bercot wrote:
> 
> >The reason I asked is that it means, if you're using TAI-10, you have
> >to have matching zoneinfo files for the daylight time transitions to
> >work correctly. This really complicates the matter of having POSIX/TAI
> >and TZ be separate...
> 
>  Yes, people using TAI-10 should also be using right/ timezones instead of
> posix/ ones. That's why TZ as a test is appealing: it ensures consistency.
> But at that level it's entirely the admin's responsibility to have
> consistent configuration: leap-second-conscious admins won't mind doing
> some extra work to have a setting that pleases them.

The problem with this approach is that an individual user may unset
$TZ or set it to a POSIX-syntax timezone such as "UTC" on a system
where the system clock is using TAI-10. Users generally expect this to
work, and their expectation is justified. Valid scripts might even use
TZ=UTC as part of their functionality.

Rich

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Szabolcs Nagy]

* Rich Felker <dalias@aerifal.cx> [2013-12-06 07:50:55 -0500]:
> On Fri, Dec 06, 2013 at 10:37:56AM +0000, Laurent Bercot wrote:
> >  Yes, people using TAI-10 should also be using right/ timezones instead of
> > posix/ ones. That's why TZ as a test is appealing: it ensures consistency.
> > But at that level it's entirely the admin's responsibility to have
> > consistent configuration: leap-second-conscious admins won't mind doing
> > some extra work to have a setting that pleases them.
> 
> The problem with this approach is that an individual user may unset
> $TZ or set it to a POSIX-syntax timezone such as "UTC" on a system
> where the system clock is using TAI-10. Users generally expect this to
> work, and their expectation is justified. Valid scripts might even use
> TZ=UTC as part of their functionality.

this just means the tz data depends on the timescale used

if an admin is willing to do the leapsecond hassle then
putting the "right" files into /usr/share/zoneinfo can
be done as well, TZ=UTC will work

the current practice of using TZ=right/foo is wrong, it
means that gmtime-localtime is not the tzoffset or gmtime
depends on TZ

ie. an admin can choose to set up different timescale, but
then leapseconds and zoneinfo must be configured in the
filesystem accordingly (and regularly updated)

Re: [PATCHv2] Add support for leap seconds in zoneinfo files

[Rich Felker]

On Fri, Dec 06, 2013 at 02:27:29PM +0100, Szabolcs Nagy wrote:
> * Rich Felker <dalias@aerifal.cx> [2013-12-06 07:50:55 -0500]:
> > On Fri, Dec 06, 2013 at 10:37:56AM +0000, Laurent Bercot wrote:
> > >  Yes, people using TAI-10 should also be using right/ timezones instead of
> > > posix/ ones. That's why TZ as a test is appealing: it ensures consistency.
> > > But at that level it's entirely the admin's responsibility to have
> > > consistent configuration: leap-second-conscious admins won't mind doing
> > > some extra work to have a setting that pleases them.
> > 
> > The problem with this approach is that an individual user may unset
> > $TZ or set it to a POSIX-syntax timezone such as "UTC" on a system
> > where the system clock is using TAI-10. Users generally expect this to
> > work, and their expectation is justified. Valid scripts might even use
> > TZ=UTC as part of their functionality.
> 
> this just means the tz data depends on the timescale used
> 
> if an admin is willing to do the leapsecond hassle then
> putting the "right" files into /usr/share/zoneinfo can
> be done as well, TZ=UTC will work

No, UTC is not a file. Since UTC matches the POSIX syntax for
timezones it's required to be interpreted as a STD[off[DST[off]]]
rather than as a filename.

Rich