[PATCH 0/4] Pull Request

[PATCH 0/4] Pull Request

[J William Piggott]

This patch set address four bugs. Three are related to ISO 8601 formats
and the fourth is a tangentially related bug in hwclock. 

Patch 0002 increases the ISO 8601 buffer macro from 32 to 42 which should
work for the first three, and may be usable in the last four files:
  login-utils/last.c:1039	buffer size 32
  misc-utils/uuidparse.c:231	uses ISO_8601_BUFSIZ + 4
  login-utils/utmpdump.c:94	buffer size 40
  login-utils/lslogins.c:316	buffer size 64
  sys-utils/lsipc.c:1328	buffer size 64
  sys-utils/dmesg.c:887		buffer size 256
  term-utils/script.c:351	uses BUFSIZ (8K on my system)

I haven't tested it on them.

The the final patch adds some common ISO timestamp format masks.

The following changes since commit b41bac08abadbea9bac7a093c995ca53d86c76f1:

  build-sys: move rfkill to /usr/sbin (2017-10-20 14:59:16 +0200)

are available in the git repository at:

  git@github.com:jwpi/util-linux.git 170925

for you to fetch changes up to a9f92c6d1f25f4111f1334bdb2dd96f8b4ccb9ba:

  lib/timeutils: add common ISO timestamp masks (2017-10-21 20:55:01 -0400)

----------------------------------------------------------------
J William Piggott (4):
      hwclock: add iso-8601 overflow check
      lib/timeutils: ISO_8601_BUFSIZ too small
      lib/timeutils: add get_gmtoff()
      lib/timeutils: add common ISO timestamp masks

 include/timeutils.h    | 23 ++++++++++++-----
 lib/timeutils.c        | 70 +++++++++++++++++++++++++++++++++++++++++++++++---
 login-utils/last.c     |  2 +-
 login-utils/lslogins.c |  3 +--
 login-utils/utmpdump.c |  4 +--
 misc-utils/uuidparse.c | 10 ++------
 sys-utils/dmesg.c      |  4 +--
 sys-utils/hwclock.c    | 23 +++++++----------
 sys-utils/lsipc.c      |  2 +-
 sys-utils/rfkill.c     |  8 ++----
 term-utils/script.c    |  8 ++----
 11 files changed, 103 insertions(+), 54 deletions(-)

[PATCH 1/4] hwclock: add iso-8601 overflow check

[J William Piggott]

hwclock wasn't testing for strtimeval_iso() truncation:

/sbin/hwclock --utc --noadjfile --predict --date '7982 years'; echo $?
9999-09-25 19:33:01.000000-0400
0

/sbin/hwclock --utc --noadjfile --predict --date '7983 years'; echo $?
10000-09-25 19:33:10.000000-
0

Patched:
./hwclock --utc --noadjfile --predict --date '7982 years'; echo $?
9999-09-25 19:22:15.000000-0400
0

./hwclock --utc --noadjfile --predict --date '7983 years'; echo $?
hwclock: iso-8601 format truncated
1

Signed-off-by: J William Piggott <elseifthen@gmx.com>
---
 sys-utils/hwclock.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 36b6204b0..c2c20812c 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -551,15 +551,19 @@ set_hardware_clock_exact(const struct hwclock_control *ctl,
 	set_hardware_clock(ctl, newhwtime);
 }
 
-static void
+static int
 display_time(struct timeval hwctime)
 {
 	char buf[ISO_8601_BUFSIZ];
 
-	strtimeval_iso(&hwctime, ISO_8601_DATE|ISO_8601_TIME|ISO_8601_DOTUSEC|
+	if (strtimeval_iso(&hwctime, ISO_8601_DATE|ISO_8601_TIME|ISO_8601_DOTUSEC|
 				 ISO_8601_TIMEZONE|ISO_8601_SPACE,
-				 buf, sizeof(buf));
+				 buf, sizeof(buf))) {
+		warnx(_("iso-8601 format overflow"));
+		return EXIT_FAILURE;
+	}
 	printf("%s\n", buf);
+	return EXIT_SUCCESS;
 }
 
 /*
@@ -931,8 +935,7 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 			printf(_ ("Target date:   %ld\n"), set_time);
 			printf(_ ("Predicted RTC: %ld\n"), hclocktime.tv_sec);
 		}
-		display_time(hclocktime);
-		return EXIT_SUCCESS;
+		return display_time(hclocktime);
 	}
 
 	if (ctl->systz)
@@ -978,7 +981,7 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 		 time_inc(hclocktime, time_diff(startup_time, read_time));
 	}
 	if (ctl->show || ctl->get) {
-		display_time(startup_hclocktime);
+		return display_time(startup_hclocktime);
 	} else if (ctl->set) {
 		set_hardware_clock_exact(ctl, set_time, startup_time);
 		if (!ctl->noadjfile)

[PATCH 2/4] lib/timeutils: ISO_8601_BUFSIZ too small

[J William Piggott]

Although iso-8601 specifies years as 4 digits, it allows
them to be wider.

The current POSIX year width is limited by 'int tm_year'
at 10 digits plus a negative sign.

That, and the possibility of nanosecond time makes the
widest POSIX iso-8601 time 41 characters. Plus the \0
string terminator yields a buffer size of 42.

Before truncated output:
/sbin/hwclock --utc --noadjfile --predict --date '-2147483765 years'
-2147481748-09-25 20:29:45.0000

Patched:
./hwclock --utc --noadjfile --predict --date '-2147483765 years'
-2147481748-09-25 20:17:21.000000-0456

./hwclock --utc --noadjfile --predict --date '-2147483766 years'
hwclock: invalid date '-2147483766 years'

Comparable to coreutils 'date' command:
date -Ins --date '-2147483765 years'
-2147481748-09-25T19:49:31,578899297-0456

date -Ins --date '-2147483766 years'
date: invalid date '-2147483766 years'

The 'date' output illustrates the full 41 character POSIX iso-8601

Signed-off-by: J William Piggott <elseifthen@gmx.com>
---
 include/timeutils.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/timeutils.h b/include/timeutils.h
index 874f853b7..edd42f7fe 100644
--- a/include/timeutils.h
+++ b/include/timeutils.h
@@ -65,7 +65,7 @@ enum {
 	ISO_8601_GMTIME		= (1 << 7)
 };
 
-#define ISO_8601_BUFSIZ	32
+#define ISO_8601_BUFSIZ	42
 
 int strtimeval_iso(struct timeval *tv, int flags, char *buf, size_t bufsz);
 int strtm_iso(struct tm *tm, int flags, char *buf, size_t bufsz);

[PATCH 3/4] lib/timeutils: add get_gmtoff()

[J William Piggott]

This new function returns the GMT offset relative to its
argument. It is used in this patch to fix two bugs:

1) On platforms that the tm struct excludes tm_gmtoff,
   hwclock assumes a one hour DST offset. This can cause
   an incorrect kernel timezone setting. For example:

 Current; with tm_gmtoff illustrates the correct offset:
$ TZ="Australia/Lord_Howe" hwclock --hctosys --test | grep settimeofday
Calling settimeofday(1507494204.192398, -660)

 Current; without tm_gmtoff:
$ TZ="Australia/Lord_Howe" hwclock --hctosys --test | grep settimeofday
Calling settimeofday(1507494249.193852, -690)

 Patched; without tm_gmtoff:
$ TZ="Australia/Lord_Howe" hwclock --hctosys --test | grep settimeofday
Calling settimeofday(1507494260.194208, -660)

2) ISO 8601 'extended' format requires all time elements
   to use a colon (:).

Current invalid ISO 8601:
$ hwclock
2017-10-08 16:25:17.895462-0400

Patched:
$ hwclock
2017-10-08 16:25:34.141895-04:00

Signed-off-by: J William Piggott <elseifthen@gmx.com>
---
 include/timeutils.h |  1 +
 lib/timeutils.c     | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 sys-utils/hwclock.c |  8 +------
 3 files changed, 68 insertions(+), 9 deletions(-)

diff --git a/include/timeutils.h b/include/timeutils.h
index edd42f7fe..e8a261462 100644
--- a/include/timeutils.h
+++ b/include/timeutils.h
@@ -53,6 +53,7 @@ typedef uint64_t nsec_t;
 #define FORMAT_TIMESPAN_MAX 64
 
 int parse_timestamp(const char *t, usec_t *usec);
+int get_gmtoff(const struct tm *tp);
 
 /* flags for strxxx_iso() functions */
 enum {
diff --git a/lib/timeutils.c b/lib/timeutils.c
index d38970c10..adc255c33 100644
--- a/lib/timeutils.c
+++ b/lib/timeutils.c
@@ -341,6 +341,65 @@ int parse_timestamp(const char *t, usec_t *usec)
 	return 0;
 }
 
+/* Returns the difference in seconds between its argument and GMT. If if TP is
+ * invalid or no DST information is available default to UTC, that is, zero.
+ * tzset is called so, for example, 'TZ="UTC" hwclock' will work as expected.
+ * Derived from glibc/time/strftime_l.c
+ */
+int get_gmtoff(const struct tm *tp)
+{
+	if (tp->tm_isdst < 0)
+	return 0;
+
+#if HAVE_TM_GMTOFF
+	return tp->tm_gmtoff;
+#else
+	struct tm tm;
+	struct tm gtm;
+	struct tm ltm = *tp;
+	time_t lt;
+
+	tzset();
+	lt = mktime(&ltm);
+	/* Check if mktime returning -1 is an error or a valid time_t */
+	if (lt == (time_t) -1) {
+		if (! localtime_r(&lt, &tm)
+			|| ((ltm.tm_sec ^ tm.tm_sec)
+			    | (ltm.tm_min ^ tm.tm_min)
+			    | (ltm.tm_hour ^ tm.tm_hour)
+			    | (ltm.tm_mday ^ tm.tm_mday)
+			    | (ltm.tm_mon ^ tm.tm_mon)
+			    | (ltm.tm_year ^ tm.tm_year)))
+			return 0;
+	}
+
+	if (! gmtime_r(&lt, &gtm))
+		return 0;
+
+	/* Calculate the GMT offset, that is, the difference between the
+	 * TP argument (ltm) and GMT (gtm).
+	 *
+	 * Compute intervening leap days correctly even if year is negative.
+	 * Take care to avoid int overflow in leap day calculations, but it's OK
+	 * to assume that A and B are close to each other.
+	 */
+	int a4 = (ltm.tm_year >> 2) + (1900 >> 2) - ! (ltm.tm_year & 3);
+	int b4 = (gtm.tm_year >> 2) + (1900 >> 2) - ! (gtm.tm_year & 3);
+	int a100 = a4 / 25 - (a4 % 25 < 0);
+	int b100 = b4 / 25 - (b4 % 25 < 0);
+	int a400 = a100 >> 2;
+	int b400 = b100 >> 2;
+	int intervening_leap_days = (a4 - b4) - (a100 - b100) + (a400 - b400);
+
+	int years = ltm.tm_year - gtm.tm_year;
+	int days = (365 * years + intervening_leap_days
+		    + (ltm.tm_yday - gtm.tm_yday));
+
+	return (60 * (60 * (24 * days + (ltm.tm_hour - gtm.tm_hour))
+		+ (ltm.tm_min - gtm.tm_min)) + (ltm.tm_sec - gtm.tm_sec));
+#endif
+}
+
 static int format_iso_time(struct tm *tm, suseconds_t usec, int flags, char *buf, size_t bufsz)
 {
 	char *p = buf;
@@ -386,9 +445,14 @@ static int format_iso_time(struct tm *tm, suseconds_t usec, int flags, char *buf
 		p += len;
 	}
 
-	if (flags & ISO_8601_TIMEZONE && strftime(p, bufsz, "%z", tm) <= 0)
+	if (flags & ISO_8601_TIMEZONE) {
+		int tmin  = get_gmtoff(tm) / 60;
+		int zhour = tmin / 60;
+		int zmin  = abs(tmin % 60);
+		len = snprintf(p, bufsz, "%+03d:%02d", zhour,zmin);
+		if (len < 0 || (size_t) len > bufsz)
 		return -1;
-
+	}
 	return 0;
 }
 
diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index c2c20812c..3ac43efee 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -608,13 +608,7 @@ set_system_clock(const struct hwclock_control *ctl,
 	const struct timezone tz_utc = { 0 };
 
 	broken = localtime(&newtime.tv_sec);
-#ifdef HAVE_TM_GMTOFF
-	minuteswest = -broken->tm_gmtoff / 60;	/* GNU extension */
-#else
-	minuteswest = timezone / 60;
-	if (broken->tm_isdst)
-		minuteswest -= 60;
-#endif
+	minuteswest = -get_gmtoff(broken) / 60;
 
 	if (ctl->debug) {
 		if (ctl->hctosys && !ctl->universal)

[PATCH 4/4] lib/timeutils: add common ISO timestamp masks

[J William Piggott]

* Start the ISO format flags at bit 0 instead of bit 1.

* ISO timestamps have date-time-timzone in common, so move
  the TIMEZONE flag to bit 2 causing all timestamp masks
  to have the first three bits set and the last four bits
  as timestamp 'options'.

* Change the 'SPACE' flag to a 'T' flag, because it makes
  the code and comments more concise.

* Add common ISO timestamp masks.

* Implement the ISO timestamp masks in all applicable code
  using the strxxx_iso() functions.

Signed-off-by: J William Piggott <elseifthen@gmx.com>
---
 include/timeutils.h    | 20 ++++++++++++++------
 lib/timeutils.c        |  2 +-
 login-utils/last.c     |  2 +-
 login-utils/lslogins.c |  3 +--
 login-utils/utmpdump.c |  4 +---
 misc-utils/uuidparse.c | 10 ++--------
 sys-utils/dmesg.c      |  4 +---
 sys-utils/hwclock.c    |  4 +---
 sys-utils/lsipc.c      |  2 +-
 sys-utils/rfkill.c     |  8 ++------
 term-utils/script.c    |  8 ++------
 11 files changed, 27 insertions(+), 40 deletions(-)

diff --git a/include/timeutils.h b/include/timeutils.h
index e8a261462..7d5b13b2d 100644
--- a/include/timeutils.h
+++ b/include/timeutils.h
@@ -55,15 +55,23 @@ typedef uint64_t nsec_t;
 int parse_timestamp(const char *t, usec_t *usec);
 int get_gmtoff(const struct tm *tp);
 
-/* flags for strxxx_iso() functions */
+/* flags and masks for strxxx_iso() functions */
 enum {
-	ISO_8601_DATE		= (1 << 1),
-	ISO_8601_TIME		= (1 << 2),
+	ISO_8601_DATE		= (1 << 0),
+	ISO_8601_TIME		= (1 << 1),
+	ISO_8601_TIMEZONE	= (1 << 2),
 	ISO_8601_DOTUSEC	= (1 << 3),
 	ISO_8601_COMMAUSEC	= (1 << 4),
-	ISO_8601_TIMEZONE	= (1 << 5),
-	ISO_8601_SPACE		= (1 << 6),
-	ISO_8601_GMTIME		= (1 << 7)
+	ISO_8601_T		= (1 << 5),
+	ISO_8601_GMTIME		= (1 << 6),
+	ISO_TIMESTAMP		= 0x07,   /* 2017-10-13 17:27:04-04:00 */
+	ISO_TIMESTAMP_T		= 0x27,   /* 2017-10-13T17:27:04-04:00 */
+	ISO_TIMESTAMP_DOT	= 0x0F,   /* 2017-10-13 17:27:04.350452-04:00 */
+	ISO_TIMESTAMP_DOT_T	= 0x2F,   /* 2017-10-13T17:27:04.350452-04:00 */
+	ISO_TIMESTAMP_COMMA	= 0x17,   /* 2017-10-13 17:27:04,350452-04:00 */
+	ISO_TIMESTAMP_COMMA_T	= 0x37,   /* 2017-10-13T17:27:04,350452-04:00 */
+	ISO_TIMESTAMP_COMMA_G	= 0x57,   /* 2017-10-13 21:27:04,350452+00:00 */
+	ISO_TIMESTAMP_COMMA_GT  = 0x77    /* 2017-10-13T21:27:04,350452+00:00 */
 };
 
 #define ISO_8601_BUFSIZ	42
diff --git a/lib/timeutils.c b/lib/timeutils.c
index adc255c33..21d384c58 100644
--- a/lib/timeutils.c
+++ b/lib/timeutils.c
@@ -417,7 +417,7 @@ static int format_iso_time(struct tm *tm, suseconds_t usec, int flags, char *buf
 	if ((flags & ISO_8601_DATE) && (flags & ISO_8601_TIME)) {
 		if (bufsz < 1)
 			return -1;
-		*p++ = (flags & ISO_8601_SPACE) ? ' ' : 'T';
+		*p++ = (flags & ISO_8601_T) ? 'T' : ' ';
 		bufsz--;
 	}
 
diff --git a/login-utils/last.c b/login-utils/last.c
index f989836ba..8d69ed54e 100644
--- a/login-utils/last.c
+++ b/login-utils/last.c
@@ -349,7 +349,7 @@ static int time_formatter(int fmt, char *dst, size_t dlen, time_t *when)
 		ret = rtrim_whitespace((unsigned char *) dst);
 		break;
 	case LAST_TIMEFTM_ISO8601:
-		ret = strtime_iso(when, ISO_8601_DATE|ISO_8601_TIME|ISO_8601_TIMEZONE, dst, dlen);
+		ret = strtime_iso(when, ISO_TIMESTAMP_T, dst, dlen);
 		break;
 	default:
 		abort();
diff --git a/login-utils/lslogins.c b/login-utils/lslogins.c
index 1042b9b41..9a55df880 100644
--- a/login-utils/lslogins.c
+++ b/login-utils/lslogins.c
@@ -333,8 +333,7 @@ static char *make_time(int mode, time_t time)
 				buf, sizeof(buf));
 		break;
 	case TIME_ISO:
-		rc = strtime_iso(&time, ISO_8601_DATE|ISO_8601_TIME|ISO_8601_TIMEZONE,
-				   buf, sizeof(buf));
+		rc = strtime_iso(&time, ISO_TIMESTAMP_T, buf, sizeof(buf));
 		break;
 	case TIME_ISO_SHORT:
 		rc = strtime_iso(&time, ISO_8601_DATE, buf, sizeof(buf));
diff --git a/login-utils/utmpdump.c b/login-utils/utmpdump.c
index 00c44b8db..5cc87834a 100644
--- a/login-utils/utmpdump.c
+++ b/login-utils/utmpdump.c
@@ -102,9 +102,7 @@ static void print_utline(struct utmpx *ut, FILE *out)
 	tv.tv_sec = ut->ut_tv.tv_sec;
 	tv.tv_usec = ut->ut_tv.tv_usec;
 
-	if (strtimeval_iso(&tv,
-			   ISO_8601_DATE | ISO_8601_TIME | ISO_8601_COMMAUSEC |
-			   ISO_8601_TIMEZONE | ISO_8601_GMTIME, time_string,
+	if (strtimeval_iso(&tv, ISO_TIMESTAMP_COMMA_GT, time_string,
 			   sizeof(time_string)) != 0)
 		return;
 	cleanse(ut->ut_id);
diff --git a/misc-utils/uuidparse.c b/misc-utils/uuidparse.c
index 08ba33415..5d2ea7810 100644
--- a/misc-utils/uuidparse.c
+++ b/misc-utils/uuidparse.c
@@ -227,14 +227,8 @@ static void fill_table_row(struct libscols_table *tb, char const *const uuid)
 				char date_buf[ISO_8601_BUFSIZ + 4];
 
 				uuid_time(buf, &tv);
-				strtimeval_iso(&tv,
-					       ISO_8601_DATE |
-						 ISO_8601_TIME |
-						 ISO_8601_COMMAUSEC |
-						 ISO_8601_TIMEZONE |
-						 ISO_8601_SPACE,
-					       date_buf,
-					       sizeof(date_buf));
+				strtimeval_iso(&tv, ISO_TIMESTAMP_COMMA,
+					       date_buf, sizeof(date_buf));
 				str = xstrdup(date_buf);
 			}
 			break;
diff --git a/sys-utils/dmesg.c b/sys-utils/dmesg.c
index b626380f7..8a86c2ccb 100644
--- a/sys-utils/dmesg.c
+++ b/sys-utils/dmesg.c
@@ -844,9 +844,7 @@ static char *iso_8601_time(struct dmesg_control *ctl, struct dmesg_record *rec,
 		.tv_usec = rec->tv.tv_usec
 	};
 
-	if (strtimeval_iso(&tv,	ISO_8601_DATE|ISO_8601_TIME|ISO_8601_COMMAUSEC|
-				ISO_8601_TIMEZONE,
-				buf, bufsz) != 0)
+	if (strtimeval_iso(&tv,	ISO_TIMESTAMP_COMMA_T, buf, bufsz) != 0)
 		return NULL;
 
 	return buf;
diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 3ac43efee..c1eb8b939 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -556,9 +556,7 @@ display_time(struct timeval hwctime)
 {
 	char buf[ISO_8601_BUFSIZ];
 
-	if (strtimeval_iso(&hwctime, ISO_8601_DATE|ISO_8601_TIME|ISO_8601_DOTUSEC|
-				 ISO_8601_TIMEZONE|ISO_8601_SPACE,
-				 buf, sizeof(buf))) {
+	if (strtimeval_iso(&hwctime, ISO_TIMESTAMP_DOT, buf, sizeof(buf))) {
 		warnx(_("iso-8601 format overflow"));
 		return EXIT_FAILURE;
 	}
diff --git a/sys-utils/lsipc.c b/sys-utils/lsipc.c
index e99c861ab..4b3b0c92d 100644
--- a/sys-utils/lsipc.c
+++ b/sys-utils/lsipc.c
@@ -451,7 +451,7 @@ static char *make_time(int mode, time_t time)
 		strtime_short(&time, &now, 0, buf, sizeof(buf));
 		break;
 	case TIME_ISO:
-		strtime_iso(&time, ISO_8601_DATE|ISO_8601_TIME|ISO_8601_TIMEZONE, buf, sizeof(buf));
+		strtime_iso(&time, ISO_TIMESTAMP_T, buf, sizeof(buf));
 		break;
 	default:
 		errx(EXIT_FAILURE, _("unsupported time type"));
diff --git a/sys-utils/rfkill.c b/sys-utils/rfkill.c
index c9559ef48..044624188 100644
--- a/sys-utils/rfkill.c
+++ b/sys-utils/rfkill.c
@@ -253,12 +253,8 @@ static int rfkill_event(void)
 			continue;
 
 		gettimeofday(&tv, NULL);
-		strtimeval_iso(&tv,
-			       ISO_8601_DATE |
-			       ISO_8601_TIME |
-			       ISO_8601_COMMAUSEC |
-			       ISO_8601_TIMEZONE |
-			       ISO_8601_SPACE, date_buf, sizeof(date_buf));
+		strtimeval_iso(&tv, ISO_TIMESTAMP_COMMA, date_buf,
+			       sizeof(date_buf));
 		printf("%s: idx %u type %u op %u soft %u hard %u\n",
 		       date_buf,
 		       event.idx, event.type, event.op, event.soft, event.hard);
diff --git a/term-utils/script.c b/term-utils/script.c
index cf63ab336..f991de14e 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -472,9 +472,7 @@ static void do_io(struct script_control *ctl)
 
 
 	if (ctl->typescriptfp) {
-		strtime_iso(&tvec, ISO_8601_DATE | ISO_8601_TIME |
-				ISO_8601_TIMEZONE | ISO_8601_SPACE,
-				buf, sizeof(buf));
+		strtime_iso(&tvec, ISO_TIMESTAMP, buf, sizeof(buf));
 		fprintf(ctl->typescriptfp, _("Script started on %s\n"), buf);
 	}
 	gettime_monotonic(&ctl->oldtime);
@@ -546,9 +544,7 @@ static void do_io(struct script_control *ctl)
 
 	if (ctl->typescriptfp) {
 		tvec = script_time((time_t *)NULL);
-		strtime_iso(&tvec, ISO_8601_DATE | ISO_8601_TIME |
-				ISO_8601_TIMEZONE | ISO_8601_SPACE,
-				buf, sizeof(buf));
+		strtime_iso(&tvec, ISO_TIMESTAMP, buf, sizeof(buf));
 		fprintf(ctl->typescriptfp, _("\nScript done on %s\n"), buf);
 	}
 	done(ctl);

Re: [PATCH 4/4] lib/timeutils: add common ISO timestamp masks

[Karel Zak]

On Sun, Oct 22, 2017 at 08:41:33PM -0400, J William Piggott wrote:
> 
> * Start the ISO format flags at bit 0 instead of bit 1.
> 
> * ISO timestamps have date-time-timzone in common, so move
>   the TIMEZONE flag to bit 2 causing all timestamp masks
>   to have the first three bits set and the last four bits
>   as timestamp 'options'.
> 
> * Change the 'SPACE' flag to a 'T' flag, because it makes
>   the code and comments more concise.
> 
> * Add common ISO timestamp masks.
> 
> * Implement the ISO timestamp masks in all applicable code
>   using the strxxx_iso() functions.
> 
> Signed-off-by: J William Piggott <elseifthen@gmx.com>
> ---
>  include/timeutils.h    | 20 ++++++++++++++------
>  lib/timeutils.c        |  2 +-
>  login-utils/last.c     |  2 +-
>  login-utils/lslogins.c |  3 +--
>  login-utils/utmpdump.c |  4 +---
>  misc-utils/uuidparse.c | 10 ++--------
>  sys-utils/dmesg.c      |  4 +---
>  sys-utils/hwclock.c    |  4 +---
>  sys-utils/lsipc.c      |  2 +-
>  sys-utils/rfkill.c     |  8 ++------
>  term-utils/script.c    |  8 ++------
>  11 files changed, 27 insertions(+), 40 deletions(-)
> 
> diff --git a/include/timeutils.h b/include/timeutils.h
> index e8a261462..7d5b13b2d 100644
> --- a/include/timeutils.h
> +++ b/include/timeutils.h
> @@ -55,15 +55,23 @@ typedef uint64_t nsec_t;
>  int parse_timestamp(const char *t, usec_t *usec);
>  int get_gmtoff(const struct tm *tp);
>  
> -/* flags for strxxx_iso() functions */
> +/* flags and masks for strxxx_iso() functions */
>  enum {
> -	ISO_8601_DATE		= (1 << 1),
> -	ISO_8601_TIME		= (1 << 2),
> +	ISO_8601_DATE		= (1 << 0),
> +	ISO_8601_TIME		= (1 << 1),
> +	ISO_8601_TIMEZONE	= (1 << 2),
>  	ISO_8601_DOTUSEC	= (1 << 3),
>  	ISO_8601_COMMAUSEC	= (1 << 4),
> -	ISO_8601_TIMEZONE	= (1 << 5),
> -	ISO_8601_SPACE		= (1 << 6),
> -	ISO_8601_GMTIME		= (1 << 7)
> +	ISO_8601_T		= (1 << 5),
> +	ISO_8601_GMTIME		= (1 << 6),
> +	ISO_TIMESTAMP		= 0x07,   /* 2017-10-13 17:27:04-04:00 */
> +	ISO_TIMESTAMP_T		= 0x27,   /* 2017-10-13T17:27:04-04:00 */
> +	ISO_TIMESTAMP_DOT	= 0x0F,   /* 2017-10-13 17:27:04.350452-04:00 */
> +	ISO_TIMESTAMP_DOT_T	= 0x2F,   /* 2017-10-13T17:27:04.350452-04:00 */
> +	ISO_TIMESTAMP_COMMA	= 0x17,   /* 2017-10-13 17:27:04,350452-04:00 */
> +	ISO_TIMESTAMP_COMMA_T	= 0x37,   /* 2017-10-13T17:27:04,350452-04:00 */
> +	ISO_TIMESTAMP_COMMA_G	= 0x57,   /* 2017-10-13 21:27:04,350452+00:00 */
> +	ISO_TIMESTAMP_COMMA_GT  = 0x77    /* 2017-10-13T21:27:04,350452+00:00 */
>  };


Would be possible to use ISO_8601_* to compose ISO_TIMESTAMP_*
macros? Your hardcoded magic 0x constants make the header file
less readable. I mean

  ISO_TIMESTAMP = ISO_8601_DATE | ISO_8601_TIME | ISO_8601_TIMEZONE

and it would be also nice to add somehow info about "usec" to the
macro name (see ISO_TIMESTAMP vs. ISO_TIMESTAMP_COMMA).

What about:

    - use "U" prefix for usec times
    - use "GM_" prefix for GMTIME

for example:

   ISO_TIMESTAMP           = ISO_8601_DATE | ISO_8601_TIME | ISO_8601_TIMEZONE,
   ISO_UTIMESTAMP_DOT      = ISO_TIMESTAMP | ISO_8601_DOTUSEC,
   ISO_UTIMESTAMP_COMMA    = ISO_TIMESTAMP | ISO_8601_ISO_8601_COMMAUSEC,
   ISO_GM_UTIMESTAMP_COMMA = ISO_UTIMESTAMP_COMMA | ISO_8601_GMTIME

and so on.

The another nice way how to describe datetime format is to use chars to
complete describe the format

 ISO_YYYYMMDD_HHMISS             = ISO_8601_DATE | ISO_8601_TIME
 ISO_YYYYMMDD_HHMISS_TZ          = ISO_YYYYMMDD_HHMISS | ISO_8601_TIMEZONE
 ISO_YYYYMMDD_HHMISS_DOT_USEC    = ISO_YYYYMMDD_HHMISS | ISO_8601_DOTUSEC
 ISO_YYYYMMDDTHHMISS_COM_USEC    = ISO_YYYYMMDD_HHMISS | ISO_8601_COMUSEC
 ISO_YYYYMMDDTHHMISS_COM_USEC_TZ = ISO_YYYYMMDDTHHMISS_COM_USEC | ISO_8601_TIMEZONE

the macro name is longer, but very descriptive and easy to extend :-)

 Karel


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

Re: [PATCH 4/4] lib/timeutils: add common ISO timestamp masks

[J William Piggott]

On 10/23/2017 05:49 AM, Karel Zak wrote:
> On Sun, Oct 22, 2017 at 08:41:33PM -0400, J William Piggott wrote:
>>
>> * Start the ISO format flags at bit 0 instead of bit 1.
>>
>> * ISO timestamps have date-time-timzone in common, so move
>>   the TIMEZONE flag to bit 2 causing all timestamp masks
>>   to have the first three bits set and the last four bits
>>   as timestamp 'options'.
>>
>> * Change the 'SPACE' flag to a 'T' flag, because it makes
>>   the code and comments more concise.
>>
>> * Add common ISO timestamp masks.
>>
>> * Implement the ISO timestamp masks in all applicable code
>>   using the strxxx_iso() functions.
>>
>> Signed-off-by: J William Piggott <elseifthen@gmx.com>
>> ---
>>  include/timeutils.h    | 20 ++++++++++++++------
>>  lib/timeutils.c        |  2 +-
>>  login-utils/last.c     |  2 +-
>>  login-utils/lslogins.c |  3 +--
>>  login-utils/utmpdump.c |  4 +---
>>  misc-utils/uuidparse.c | 10 ++--------
>>  sys-utils/dmesg.c      |  4 +---
>>  sys-utils/hwclock.c    |  4 +---
>>  sys-utils/lsipc.c      |  2 +-
>>  sys-utils/rfkill.c     |  8 ++------
>>  term-utils/script.c    |  8 ++------
>>  11 files changed, 27 insertions(+), 40 deletions(-)
>>
>> diff --git a/include/timeutils.h b/include/timeutils.h
>> index e8a261462..7d5b13b2d 100644
>> --- a/include/timeutils.h
>> +++ b/include/timeutils.h
>> @@ -55,15 +55,23 @@ typedef uint64_t nsec_t;
>>  int parse_timestamp(const char *t, usec_t *usec);
>>  int get_gmtoff(const struct tm *tp);
>>  
>> -/* flags for strxxx_iso() functions */
>> +/* flags and masks for strxxx_iso() functions */
>>  enum {
>> -	ISO_8601_DATE		= (1 << 1),
>> -	ISO_8601_TIME		= (1 << 2),
>> +	ISO_8601_DATE		= (1 << 0),
>> +	ISO_8601_TIME		= (1 << 1),
>> +	ISO_8601_TIMEZONE	= (1 << 2),
>>  	ISO_8601_DOTUSEC	= (1 << 3),
>>  	ISO_8601_COMMAUSEC	= (1 << 4),
>> -	ISO_8601_TIMEZONE	= (1 << 5),
>> -	ISO_8601_SPACE		= (1 << 6),
>> -	ISO_8601_GMTIME		= (1 << 7)
>> +	ISO_8601_T		= (1 << 5),
>> +	ISO_8601_GMTIME		= (1 << 6),
>> +	ISO_TIMESTAMP		= 0x07,   /* 2017-10-13 17:27:04-04:00 */
>> +	ISO_TIMESTAMP_T		= 0x27,   /* 2017-10-13T17:27:04-04:00 */
>> +	ISO_TIMESTAMP_DOT	= 0x0F,   /* 2017-10-13 17:27:04.350452-04:00 */
>> +	ISO_TIMESTAMP_DOT_T	= 0x2F,   /* 2017-10-13T17:27:04.350452-04:00 */
>> +	ISO_TIMESTAMP_COMMA	= 0x17,   /* 2017-10-13 17:27:04,350452-04:00 */
>> +	ISO_TIMESTAMP_COMMA_T	= 0x37,   /* 2017-10-13T17:27:04,350452-04:00 */
>> +	ISO_TIMESTAMP_COMMA_G	= 0x57,   /* 2017-10-13 21:27:04,350452+00:00 */
>> +	ISO_TIMESTAMP_COMMA_GT  = 0x77    /* 2017-10-13T21:27:04,350452+00:00 */
>>  };
> 
> 
> Would be possible to use ISO_8601_* to compose ISO_TIMESTAMP_*
> macros? Your hardcoded magic 0x constants make the header file
> less readable. I mean
> 
>   ISO_TIMESTAMP = ISO_8601_DATE | ISO_8601_TIME | ISO_8601_TIMEZONE

We could eliminate wrapping by dropping the unnecessary 8601. Using the
longest example:

	ISO_TIMESTAMP_COMMA_GT = ISO_TIMESTAMP_COMMA | ISO_GMTIME | ISO_T

> 
> and it would be also nice to add somehow info about "usec" to the
> macro name (see ISO_TIMESTAMP vs. ISO_TIMESTAMP_COMMA).

8601 only uses comma or dot for the radix character; so 'COMMA/DOT'
imply the presence of a fractional part.

> What about:
> 
>     - use "U" prefix for usec times
>     - use "GM_" prefix for GMTIME
> 
> for example:
> 
>    ISO_TIMESTAMP           = ISO_8601_DATE | ISO_8601_TIME | ISO_8601_TIMEZONE,
>    ISO_UTIMESTAMP_DOT      = ISO_TIMESTAMP | ISO_8601_DOTUSEC,
>    ISO_UTIMESTAMP_COMMA    = ISO_TIMESTAMP | ISO_8601_ISO_8601_COMMAUSEC,
>    ISO_GM_UTIMESTAMP_COMMA = ISO_UTIMESTAMP_COMMA | ISO_8601_GMTIME
> 
> and so on.

IMO the macros are easier to remember, write, and read, if they all
begin the same and the optional parts are suffixes. All of the masks are
basic ISO Timestamps (date time timezone), the rest are minor variations.

> 
> The another nice way how to describe datetime format is to use chars to
> complete describe the format
> 
>  ISO_YYYYMMDD_HHMISS             = ISO_8601_DATE | ISO_8601_TIME
>  ISO_YYYYMMDD_HHMISS_TZ          = ISO_YYYYMMDD_HHMISS | ISO_8601_TIMEZONE
>  ISO_YYYYMMDD_HHMISS_DOT_USEC    = ISO_YYYYMMDD_HHMISS | ISO_8601_DOTUSEC
>  ISO_YYYYMMDDTHHMISS_COM_USEC    = ISO_YYYYMMDD_HHMISS | ISO_8601_COMUSEC
>  ISO_YYYYMMDDTHHMISS_COM_USEC_TZ = ISO_YYYYMMDDTHHMISS_COM_USEC | ISO_8601_TIMEZONE
> 
> the macro name is longer, but very descriptive and easy to extend :-)

That notation works well when necessary, but an 8601 timestamp is so
specific that duplicating it in the macro name is redundant and, I
think, visually mesmerizing.

> 
>  Karel
> 
> 

Re: [PATCH 4/4] lib/timeutils: add common ISO timestamp masks

[Karel Zak]

On Mon, Oct 23, 2017 at 03:54:19PM -0400, J William Piggott wrote:
> > Would be possible to use ISO_8601_* to compose ISO_TIMESTAMP_*
> > macros? Your hardcoded magic 0x constants make the header file
> > less readable. I mean
> > 
> >   ISO_TIMESTAMP = ISO_8601_DATE | ISO_8601_TIME | ISO_8601_TIMEZONE
> 
> We could eliminate wrapping by dropping the unnecessary 8601. Using the
> longest example:
> 
> 	ISO_TIMESTAMP_COMMA_GT = ISO_TIMESTAMP_COMMA | ISO_GMTIME | ISO_T
 
OK, although ISO_T seems pretty generic :-)

> > and it would be also nice to add somehow info about "usec" to the
> > macro name (see ISO_TIMESTAMP vs. ISO_TIMESTAMP_COMMA).
> 
> 8601 only uses comma or dot for the radix character; so 'COMMA/DOT'
> imply the presence of a fractional part.
> 
> > What about:
> > 
> >     - use "U" prefix for usec times
> >     - use "GM_" prefix for GMTIME
> > 
> > for example:
> > 
> >    ISO_TIMESTAMP           = ISO_8601_DATE | ISO_8601_TIME | ISO_8601_TIMEZONE,
> >    ISO_UTIMESTAMP_DOT      = ISO_TIMESTAMP | ISO_8601_DOTUSEC,
> >    ISO_UTIMESTAMP_COMMA    = ISO_TIMESTAMP | ISO_8601_ISO_8601_COMMAUSEC,
> >    ISO_GM_UTIMESTAMP_COMMA = ISO_UTIMESTAMP_COMMA | ISO_8601_GMTIME
> > 
> > and so on.
> 
> IMO the macros are easier to remember, write, and read, if they all
> begin the same and the optional parts are suffixes. All of the masks are
> basic ISO Timestamps (date time timezone), the rest are minor variations.

I have a problem if I have to use more suffixes, the order of the
suffixes is not obvious. The idea behind ISO_8601_* with in code has
been to keep it readable:

    strtime_iso(ISO_8601_DATE | ISO_8601_TIME | ISO_8601_TIMEZONE)

is so easy to read...

    Karel

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

Re: [PATCH 4/4] lib/timeutils: add common ISO timestamp masks

[J William Piggott]

On 10/24/2017 05:37 AM, Karel Zak wrote:
> On Mon, Oct 23, 2017 at 03:54:19PM -0400, J William Piggott wrote:
>>> Would be possible to use ISO_8601_* to compose ISO_TIMESTAMP_*
>>> macros? Your hardcoded magic 0x constants make the header file
>>> less readable. I mean
>>>
>>>   ISO_TIMESTAMP = ISO_8601_DATE | ISO_8601_TIME | ISO_8601_TIMEZONE
>>
>> We could eliminate wrapping by dropping the unnecessary 8601. Using the
>> longest example:
>>
>> 	ISO_TIMESTAMP_COMMA_GT = ISO_TIMESTAMP_COMMA | ISO_GMTIME | ISO_T
>  
> OK, although ISO_T seems pretty generic :-)
> 
>>> and it would be also nice to add somehow info about "usec" to the
>>> macro name (see ISO_TIMESTAMP vs. ISO_TIMESTAMP_COMMA).
>>
>> 8601 only uses comma or dot for the radix character; so 'COMMA/DOT'
>> imply the presence of a fractional part.
>>
>>> What about:
>>>
>>>     - use "U" prefix for usec times
>>>     - use "GM_" prefix for GMTIME
>>>
>>> for example:
>>>
>>>    ISO_TIMESTAMP           = ISO_8601_DATE | ISO_8601_TIME | ISO_8601_TIMEZONE,
>>>    ISO_UTIMESTAMP_DOT      = ISO_TIMESTAMP | ISO_8601_DOTUSEC,
>>>    ISO_UTIMESTAMP_COMMA    = ISO_TIMESTAMP | ISO_8601_ISO_8601_COMMAUSEC,
>>>    ISO_GM_UTIMESTAMP_COMMA = ISO_UTIMESTAMP_COMMA | ISO_8601_GMTIME
>>>
>>> and so on.
>>
>> IMO the macros are easier to remember, write, and read, if they all
>> begin the same and the optional parts are suffixes. All of the masks are
>> basic ISO Timestamps (date time timezone), the rest are minor variations.
> 
> I have a problem if I have to use more suffixes, the order of the
> suffixes is not obvious. The idea behind ISO_8601_* with in code has
> been to keep it readable:

>From a readability standpoint the order doesn't matter; the order is set
by the standard. The reader only needs to know what the suffixes mean.
There are only four: COMMA, DOT, T, and G. Their meanings seem clear to me.

The current implementation puts the items out of order as well, for
example uuidparse.c has: 
ISO_8601_DATE |
ISO_8601_TIME |
ISO_8601_COMMAUSEC |
ISO_8601_TIMEZONE |
ISO_8601_SPACE,

SPACE is out of order, but it doesn't matter. The standard defines
where it goes. As long as the reader understands that SPACE defines the
date and time delimiter all is good.

Anyway, I'm not clear on what you want me to do. You didn't comment on
my idea that naming the radix point implies usec. Why define a radix
point if there is no fractional part, it makes no sense. If you agree
with that, then your only remaining change request is moving the _G
suffix to _GM prefix.  Is that the only change you want? If yes, then
I'll make it so. IMO, it degrades readability, but you have the hammer.


> 
>     strtime_iso(ISO_8601_DATE | ISO_8601_TIME | ISO_8601_TIMEZONE)
> 
> is so easy to read...
> 
>     Karel
> 

Re: [PATCH 4/4] lib/timeutils: add common ISO timestamp masks

[Karel Zak]

On Tue, Oct 24, 2017 at 12:19:59PM -0400, J William Piggott wrote:
> Anyway, I'm not clear on what you want me to do. You didn't comment on
> my idea that naming the radix point implies usec.

Yes, it implies usec, the question is if such thing is obvious for a
random reader.

> Why define a radix
> point if there is no fractional part, it makes no sense. If you agree
> with that, then your only remaining change request is moving the _G
> suffix to _GM prefix.  Is that the only change you want? If yes, then
> I'll make it so. IMO, it degrades readability, but you have the hammer.

Just brainstorming... the current situation is not so bad from my point 
of view :-)

Anyway I don't see a problem to support complete macros ISO_TIMESTAMP_
as you suggested, but please keep it without magic number with in the
header files. The _G suffix is fine. Send patch.

It would be also nice to use all the complex macros in lib/timeutils.c
test program.

    Karel

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