[PATCH] hwclock: remove UTC-0 localization hack

[PATCH] hwclock: remove UTC-0 localization hack

[Sami Kerola]

There is no need to re-implement timegm() by removing TZ localization for a
moment, just use timegm(3) instead.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/hwclock.c | 34 +++++-----------------------------
 1 file changed, 5 insertions(+), 29 deletions(-)

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index e98c2c0..bcaab8b 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -388,27 +388,11 @@ static void
 mktime_tz(struct tm tm, const bool universal,
 	  bool * valid_p, time_t * systime_p)
 {
-	time_t mktime_result;	/* The value returned by our mktime() call */
-	char *zone;		/* Local time zone name */
-
-	/*
-	 * We use the C library function mktime(), but since it only works
-	 * on local time zone input, we may have to fake it out by
-	 * temporarily changing the local time zone to UTC.
-	 */
-	zone = getenv("TZ");	/* remember original time zone */
-	if (universal) {
-		/* Set timezone to UTC */
-		setenv("TZ", "", TRUE);
-		/*
-		 * Note: tzset() gets called implicitly by the time code,
-		 * but only the first time. When changing the environment
-		 * variable, better call tzset() explicitly.
-		 */
-		tzset();
-	}
-	mktime_result = mktime(&tm);
-	if (mktime_result == -1) {
+	if (universal)
+		*systime_p = timegm(&tm);
+	else
+		*systime_p = mktime(&tm);
+	if (*systime_p == -1) {
 		/*
 		 * This apparently (not specified in mktime() documentation)
 		 * means the 'tm' structure does not contain valid values
@@ -416,7 +400,6 @@ mktime_tz(struct tm tm, const bool universal,
 		 * mktime() returns -1).
 		 */
 		*valid_p = FALSE;
-		*systime_p = 0;
 		if (debug)
 			printf(_("Invalid values in hardware clock: "
 				 "%4d/%.2d/%.2d %.2d:%.2d:%.2d\n"),
@@ -424,7 +407,6 @@ mktime_tz(struct tm tm, const bool universal,
 			       tm.tm_hour, tm.tm_min, tm.tm_sec);
 	} else {
 		*valid_p = TRUE;
-		*systime_p = mktime_result;
 		if (debug)
 			printf(_
 			       ("Hw clock time : %4d/%.2d/%.2d %.2d:%.2d:%.2d = "
@@ -432,12 +414,6 @@ mktime_tz(struct tm tm, const bool universal,
 			       tm.tm_mon + 1, tm.tm_mday, tm.tm_hour, tm.tm_min,
 			       tm.tm_sec, (long)*systime_p);
 	}
-	/* now put back the original zone. */
-	if (zone)
-		setenv("TZ", zone, TRUE);
-	else
-		unsetenv("TZ");
-	tzset();
 }
 
 /*
-- 
2.9.0

Re: [PATCH] hwclock: remove UTC-0 localization hack

[J William Piggott]

We use mktime(3) for portability.

http://www.gnu.org/software/libc/manual/html_mono/libc.html#index-timegm

Portability note: mktime is essentially universally available. timegm is
rather rare. For the most portable conversion from a UTC broken-down
time to a simple time, set the TZ environment variable to UTC, call
mktime, then set TZ back. 

On 07/10/2016 04:02 PM, Sami Kerola wrote:
> There is no need to re-implement timegm() by removing TZ localization for a
> moment, just use timegm(3) instead.
> 
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  sys-utils/hwclock.c | 34 +++++-----------------------------
>  1 file changed, 5 insertions(+), 29 deletions(-)
> 
> diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
> index e98c2c0..bcaab8b 100644
> --- a/sys-utils/hwclock.c
> +++ b/sys-utils/hwclock.c
> @@ -388,27 +388,11 @@ static void
>  mktime_tz(struct tm tm, const bool universal,
>  	  bool * valid_p, time_t * systime_p)
>  {
> -	time_t mktime_result;	/* The value returned by our mktime() call */
> -	char *zone;		/* Local time zone name */
> -
> -	/*
> -	 * We use the C library function mktime(), but since it only works
> -	 * on local time zone input, we may have to fake it out by
> -	 * temporarily changing the local time zone to UTC.
> -	 */
> -	zone = getenv("TZ");	/* remember original time zone */
> -	if (universal) {
> -		/* Set timezone to UTC */
> -		setenv("TZ", "", TRUE);
> -		/*
> -		 * Note: tzset() gets called implicitly by the time code,
> -		 * but only the first time. When changing the environment
> -		 * variable, better call tzset() explicitly.
> -		 */
> -		tzset();
> -	}
> -	mktime_result = mktime(&tm);
> -	if (mktime_result == -1) {
> +	if (universal)
> +		*systime_p = timegm(&tm);
> +	else
> +		*systime_p = mktime(&tm);
> +	if (*systime_p == -1) {
>  		/*
>  		 * This apparently (not specified in mktime() documentation)
>  		 * means the 'tm' structure does not contain valid values
> @@ -416,7 +400,6 @@ mktime_tz(struct tm tm, const bool universal,
>  		 * mktime() returns -1).
>  		 */
>  		*valid_p = FALSE;
> -		*systime_p = 0;
>  		if (debug)
>  			printf(_("Invalid values in hardware clock: "
>  				 "%4d/%.2d/%.2d %.2d:%.2d:%.2d\n"),
> @@ -424,7 +407,6 @@ mktime_tz(struct tm tm, const bool universal,
>  			       tm.tm_hour, tm.tm_min, tm.tm_sec);
>  	} else {
>  		*valid_p = TRUE;
> -		*systime_p = mktime_result;
>  		if (debug)
>  			printf(_
>  			       ("Hw clock time : %4d/%.2d/%.2d %.2d:%.2d:%.2d = "
> @@ -432,12 +414,6 @@ mktime_tz(struct tm tm, const bool universal,
>  			       tm.tm_mon + 1, tm.tm_mday, tm.tm_hour, tm.tm_min,
>  			       tm.tm_sec, (long)*systime_p);
>  	}
> -	/* now put back the original zone. */
> -	if (zone)
> -		setenv("TZ", zone, TRUE);
> -	else
> -		unsetenv("TZ");
> -	tzset();
>  }
>  
>  /*
> 

Re: [PATCH] hwclock: remove UTC-0 localization hack

[Sami Kerola]

On 11 July 2016 at 21:23, J William Piggott <elseifthen@gmx.com> wrote:
>
> We use mktime(3) for portability.
>
> http://www.gnu.org/software/libc/manual/html_mono/libc.html#index-timegm
>
> Portability note: mktime is essentially universally available. timegm is
> rather rare. For the most portable conversion from a UTC broken-down
> time to a simple time, set the TZ environment variable to UTC, call
> mktime, then set TZ back.

I don't think hwclock(8) is portable. It has ioctl(2) calls, and such.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH] hwclock: remove UTC-0 localization hack

[Ruediger Meier]

On Tuesday 12 July 2016, Sami Kerola wrote:
> On 11 July 2016 at 21:23, J William Piggott <elseifthen@gmx.com> 
wrote:
> > We use mktime(3) for portability.
> >
> > http://www.gnu.org/software/libc/manual/html_mono/libc.html#index-t
> >imegm
> >
> > Portability note: mktime is essentially universally available.
> > timegm is rather rare. For the most portable conversion from a UTC
> > broken-down time to a simple time, set the TZ environment variable
> > to UTC, call mktime, then set TZ back.
>
> I don't think hwclock(8) is portable. It has ioctl(2) calls, and
> such.

But there are non-gnu libc's for Linux.

cu,
Rudi

Re: [PATCH] hwclock: remove UTC-0 localization hack

[Sami Kerola]

On 12 July 2016 at 15:23, Ruediger Meier <sweet_f_a@gmx.de> wrote:
> On Tuesday 12 July 2016, Sami Kerola wrote:
>> On 11 July 2016 at 21:23, J William Piggott <elseifthen@gmx.com>
> wrote:
>> > We use mktime(3) for portability.
>> >
>> > http://www.gnu.org/software/libc/manual/html_mono/libc.html#index-t
>> >imegm
>> >
>> > Portability note: mktime is essentially universally available.
>> > timegm is rather rare. For the most portable conversion from a UTC
>> > broken-down time to a simple time, set the TZ environment variable
>> > to UTC, call mktime, then set TZ back.
>>
>> I don't think hwclock(8) is portable. It has ioctl(2) calls, and
>> such.
>
> But there are non-gnu libc's for Linux.

Fair point, but can you name one that does not have timegm().

https://github.com/cloudius-systems/musl/blob/master/src/time/timegm.c
https://github.com/ensc/dietlibc/blob/master/libugly/timegm.c
http://www.uclibc-ng.org/browser/uclibc-ng/libc/misc/time/timegm.c?order=name

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH] hwclock: remove UTC-0 localization hack

[Ruediger Meier]

On Tuesday 12 July 2016, Sami Kerola wrote:
> On 12 July 2016 at 15:23, Ruediger Meier <sweet_f_a@gmx.de> wrote:
> > On Tuesday 12 July 2016, Sami Kerola wrote:
> >> On 11 July 2016 at 21:23, J William Piggott <elseifthen@gmx.com>
> >
> > wrote:
> >> > We use mktime(3) for portability.
> >> >
> >> > http://www.gnu.org/software/libc/manual/html_mono/libc.html#inde
> >> >x-t imegm
> >> >
> >> > Portability note: mktime is essentially universally available.
> >> > timegm is rather rare. For the most portable conversion from a
> >> > UTC broken-down time to a simple time, set the TZ environment
> >> > variable to UTC, call mktime, then set TZ back.
> >>
> >> I don't think hwclock(8) is portable. It has ioctl(2) calls, and
> >> such.
> >
> > But there are non-gnu libc's for Linux.
>
> Fair point, but can you name one that does not have timegm().
>
> https://github.com/cloudius-systems/musl/blob/master/src/time/timegm.
>c https://github.com/ensc/dietlibc/blob/master/libugly/timegm.c
> http://www.uclibc-ng.org/browser/uclibc-ng/libc/misc/time/timegm.c?or
>der=name


Yep, also musl has timegm. But at least it's good that William reminded 
us to check that! :)

cu,
Rudi

Re: [PATCH] hwclock: remove UTC-0 localization hack

[Sami Kerola]

On Tue, 12 Jul 2016, Ruediger Meier wrote:

> On Tuesday 12 July 2016, Sami Kerola wrote:
> > On 12 July 2016 at 15:23, Ruediger Meier <sweet_f_a@gmx.de> wrote:
> > > On Tuesday 12 July 2016, Sami Kerola wrote:
> > >> On 11 July 2016 at 21:23, J William Piggott <elseifthen@gmx.com>
> > >
> > > wrote:
> > >> > We use mktime(3) for portability.
> > >> >
> > >> > http://www.gnu.org/software/libc/manual/html_mono/libc.html#inde
> > >> >x-t imegm
> > >> >
> > >> > Portability note: mktime is essentially universally available.
> > >> > timegm is rather rare. For the most portable conversion from a
> > >> > UTC broken-down time to a simple time, set the TZ environment
> > >> > variable to UTC, call mktime, then set TZ back.
> > >>
> > >> I don't think hwclock(8) is portable. It has ioctl(2) calls, and
> > >> such.
> > >
> > > But there are non-gnu libc's for Linux.
> >
> > Fair point, but can you name one that does not have timegm().
> >
> > https://github.com/cloudius-systems/musl/blob/master/src/time/timegm.
> >c https://github.com/ensc/dietlibc/blob/master/libugly/timegm.c
> > http://www.uclibc-ng.org/browser/uclibc-ng/libc/misc/time/timegm.c?or
> >der=name
> 
> 
> Yep, also musl has timegm. But at least it's good that William reminded 
> us to check that! :)

Oh well. I guess it does not hurt to support incomplete libraries even if 
we might not be able to name them.

https://github.com/kerolasa/lelux-utiliteetit/commit/5dff764bb38f0745a8bb128337d2534f109a37c8

and the same as email body so that review is possible.

--->8----
From: Sami Kerola <kerolasa@iki.fi>
Date: Tue, 12 Jul 2016 22:21:10 +0100
Subject: [PATCH] lib: add portability functions, start with timegm()

The lib/portability.c is a file where functions that may be missing from
libraries can be added.

CC: Ruediger Meier <ruediger.meier@ga-group.nl>
CC: J William Piggott <elseifthen@gmx.com>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 configure.ac           |  1 +
 include/Makemodule.am  |  1 +
 include/portability.h  |  8 ++++++++
 lib/Makemodule.am      |  1 +
 lib/portability.c      | 28 ++++++++++++++++++++++++++++
 login-utils/utmpdump.c |  1 +
 sys-utils/hwclock.c    |  1 +
 7 files changed, 41 insertions(+)
 create mode 100644 include/portability.h
 create mode 100644 lib/portability.c

diff --git a/configure.ac b/configure.ac
index 39d9aca..0835838 100644
--- a/configure.ac
+++ b/configure.ac
@@ -413,6 +413,7 @@ AC_CHECK_FUNCS([ \
 	strnlen \
 	sysconf \
 	sysinfo \
+	timegm \
 	updwtmp \
 	usleep \
 	warn \
diff --git a/include/Makemodule.am b/include/Makemodule.am
index 5ceff0a..9eff5c8 100644
--- a/include/Makemodule.am
+++ b/include/Makemodule.am
@@ -35,6 +35,7 @@ dist_noinst_HEADERS += \
 	include/path.h \
 	include/pathnames.h \
 	include/plymouth-ctrl.h \
+	include/portability.h \
 	include/procutils.h \
 	include/pt-bsd.h \
 	include/pt-mbr.h \
diff --git a/include/portability.h b/include/portability.h
new file mode 100644
index 0000000..2a60ede
--- /dev/null
+++ b/include/portability.h
@@ -0,0 +1,8 @@
+#ifndef UTIL_LINUX_PORTABILITY_H
+#define UTIL_LINUX_PORTABILITY_H
+
+#ifndef HAVE_TIMEGM
+extern time_t timegm(struct tm *tm);
+#endif
+
+#endif /* UTIL_LINUX_PORTABILITY_H */
diff --git a/lib/Makemodule.am b/lib/Makemodule.am
index 704a16e..57b1d86 100644
--- a/lib/Makemodule.am
+++ b/lib/Makemodule.am
@@ -16,6 +16,7 @@ libcommon_la_SOURCES = \
 	lib/md5.c \
 	lib/pager.c \
 	lib/path.c \
+	lib/portability.c \
 	lib/randutils.c \
 	lib/setproctitle.c \
 	lib/strutils.c \
diff --git a/lib/portability.c b/lib/portability.c
new file mode 100644
index 0000000..cbc49a5
--- /dev/null
+++ b/lib/portability.c
@@ -0,0 +1,28 @@
+/*
+ * Functions in this file are replacements when library functions neither
+ * unimplemented or unavailable due old version.  In best case these
+ * functions are never needed.
+ */
+
+#include <time.h>
+#include <stdlib.h>
+
+#include "portability.h"
+
+#ifndef HAVE_TIMEGM
+time_t timegm(struct tm *tm)
+{
+	const char *zone = getenv("TZ");
+	time_t ret;
+
+	setenv("TZ", "", 1);
+	tzset();
+	ret = mktime(tm);
+	if (zone)
+		setenv("TZ", zone, 1);
+	else
+		unsetenv("TZ");
+	tzset();
+	return ret;
+}
+#endif /* HAVE_TIMEGM */
diff --git a/login-utils/utmpdump.c b/login-utils/utmpdump.c
index 1849a4e..43fc842 100644
--- a/login-utils/utmpdump.c
+++ b/login-utils/utmpdump.c
@@ -45,6 +45,7 @@
 #include "timeutils.h"
 #include "xalloc.h"
 #include "closestream.h"
+#include "portability.h"
 
 static time_t strtotime(const char *s_time)
 {
diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index bcaab8b..fb9d628 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -78,6 +78,7 @@
 #include "nls.h"
 #include "optutils.h"
 #include "pathnames.h"
+#include "portability.h"
 #include "strutils.h"
 #include "hwclock.h"
 #include "timeutils.h"
-- 
2.9.0

Re: [PATCH] hwclock: remove UTC-0 localization hack

[Karel Zak]

On Tue, Jul 12, 2016 at 10:42:33PM +0100, Sami Kerola wrote:
>  lib/portability.c      | 28 ++++++++++++++++++++++++++++

We already care about portability and we have many fallbacks, so why
we need lib/portability.c now? It would better to use timeutils.c (or
inline function in c.h).

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] hwclock: remove UTC-0 localization hack

[Sami Kerola]

On 14 July 2016 at 11:01, Karel Zak <kzak@redhat.com> wrote:
> On Tue, Jul 12, 2016 at 10:42:33PM +0100, Sami Kerola wrote:
>>  lib/portability.c      | 28 ++++++++++++++++++++++++++++
>
> We already care about portability and we have many fallbacks, so why
> we need lib/portability.c now? It would better to use timeutils.c (or
> inline function in c.h).

Hi Karel.

I didn't do my homework carefully enough. After assuming the timeutils.c
should be kept close to simiar with systemd version I thought some other
file should be used. But now when I look systemd version of the time-utils.c
and compare it with the util-linux copy it's pretty clear these are two very
different pieces of code. Change below moved the replacement function to
the timeutils.c with necessary commit message update.

https://github.com/kerolasa/lelux-utiliteetit/commit/0f916063a4f887f198e005a7a0f7878aa0bfbe05

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH] hwclock: remove UTC-0 localization hack

[J William Piggott]

On 07/14/2016 06:01 AM, Karel Zak wrote:
> On Tue, Jul 12, 2016 at 10:42:33PM +0100, Sami Kerola wrote:
>>  lib/portability.c      | 28 ++++++++++++++++++++++++++++
> 
> We already care about portability and we have many fallbacks, so why
> we need lib/portability.c now? It would better to use timeutils.c (or
> inline function in c.h).
> 
>     Karel
> 
>

What is this projects position on POSIX compatibility? A few comments in
the *-ReleaseNotes is all I found in /Documentation. Just curious, because
mktime() is and timegm() is not POSIX.

Re: [PATCH] hwclock: remove UTC-0 localization hack

[Sami Kerola]

On 15 July 2016 at 19:46, J William Piggott <elseifthen@gmx.com> wrote:
> On 07/14/2016 06:01 AM, Karel Zak wrote:
>> On Tue, Jul 12, 2016 at 10:42:33PM +0100, Sami Kerola wrote:
>>>  lib/portability.c      | 28 ++++++++++++++++++++++++++++
>>
>> We already care about portability and we have many fallbacks, so why
>> we need lib/portability.c now? It would better to use timeutils.c (or
>> inline function in c.h).
>
> What is this projects position on POSIX compatibility? A few comments in
> the *-ReleaseNotes is all I found in /Documentation. Just curious, because
> mktime() is and timegm() is not POSIX.

That is a question to maintainer, but Karel seems to be busy so let me try
to phrase how I see the things.

The util-linux project is a dumping ground for various linux specific, and
other tools.  Some of the tools make are sensible only in context of linux,
such as dmesg(1) or mkswap(8), while other could technically be compiled on
other operating environments, more(1) and getopt(1) are examples of later
category.

As mentioned in this thread the util-linux is one of the core packages, and
it is assumed to be found from all sorts of systems.  That in mind the
portability question has got more to do whether the project works without
problems with alternative libc implementations.  Ideally the util-linux
should compile fine on any linux no matter what libc is used.  But please
notice that some utilities might not get compiled if/when autotools notice
requirements are not fulfilled.

In short.  While portability is not a main goal of the project neither it is
completely neglected either.  Keeping ulibc going is important, making the
util-linux work with mingw is less important.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH] hwclock: remove UTC-0 localization hack

[J William Piggott]

On 07/27/2016 12:07 PM, Sami Kerola wrote:
> On 15 July 2016 at 19:46, J William Piggott <elseifthen@gmx.com> wrote:
>> On 07/14/2016 06:01 AM, Karel Zak wrote:
>>> On Tue, Jul 12, 2016 at 10:42:33PM +0100, Sami Kerola wrote:
>>>>  lib/portability.c      | 28 ++++++++++++++++++++++++++++
>>>
>>> We already care about portability and we have many fallbacks, so why
>>> we need lib/portability.c now? It would better to use timeutils.c (or
>>> inline function in c.h).
>>
>> What is this projects position on POSIX compatibility? A few comments in
>> the *-ReleaseNotes is all I found in /Documentation. Just curious, because
>> mktime() is and timegm() is not POSIX.
> 
> That is a question to maintainer, but Karel seems to be busy so let me try
> to phrase how I see the things.
> 
> The util-linux project is a dumping ground for various linux specific, and
> other tools.  Some of the tools make are sensible only in context of linux,
> such as dmesg(1) or mkswap(8), while other could technically be compiled on
> other operating environments, more(1) and getopt(1) are examples of later
> category.
> 
> As mentioned in this thread the util-linux is one of the core packages, and
> it is assumed to be found from all sorts of systems.  That in mind the
> portability question has got more to do whether the project works without
> problems with alternative libc implementations.  Ideally the util-linux
> should compile fine on any linux no matter what libc is used.  But please
> notice that some utilities might not get compiled if/when autotools notice
> requirements are not fulfilled.
> 
> In short.  While portability is not a main goal of the project neither it is
> completely neglected either.  Keeping ulibc going is important, making the
> util-linux work with mingw is less important.
> 

Hey Sami,

Thanks for your reply. The position you describe is what I had surmised
as a general statement (with the addition that portability also involves
different hardware architectures). I just wanted to know if Karel had an
official stance specifically on POSIX compliance. POSIX appears to only
be used as an ad hoc reference?

Re: [PATCH] hwclock: remove UTC-0 localization hack

[Karel Zak]

On Wed, Jul 27, 2016 at 05:07:38PM +0100, Sami Kerola wrote:
> On 15 July 2016 at 19:46, J William Piggott <elseifthen@gmx.com> wrote:
> > On 07/14/2016 06:01 AM, Karel Zak wrote:
> >> On Tue, Jul 12, 2016 at 10:42:33PM +0100, Sami Kerola wrote:
> >>>  lib/portability.c      | 28 ++++++++++++++++++++++++++++
> >>
> >> We already care about portability and we have many fallbacks, so why
> >> we need lib/portability.c now? It would better to use timeutils.c (or
> >> inline function in c.h).
> >
> > What is this projects position on POSIX compatibility? A few comments in
> > the *-ReleaseNotes is all I found in /Documentation. Just curious, because
> > mktime() is and timegm() is not POSIX.
> 
> That is a question to maintainer, but Karel seems to be busy so let me try
> to phrase how I see the things.

I had vacation last 2 weeks.
 
> As mentioned in this thread the util-linux is one of the core packages, and
> it is assumed to be found from all sorts of systems.  That in mind the
> portability question has got more to do whether the project works without
> problems with alternative libc implementations.  Ideally the util-linux
> should compile fine on any linux no matter what libc is used.  But please
> notice that some utilities might not get compiled if/when autotools notice
> requirements are not fulfilled.
> 
> In short.  While portability is not a main goal of the project neither it is
> completely neglected either.  Keeping ulibc going is important, making the
> util-linux work with mingw is less important.
 
Yes.

The primary goal is Linux with all usable libc, the another operation systems 
and exotic libraries are supported only if the support has no negative impact 
to the Linux use-cases and code maintenance. 

As in another cases we assume common sense rather than any strict rules ;-)

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com