[PATCH 0/3] hwclock: remove date(1)

[PATCH 0/3] hwclock: remove date(1)

[J William Piggott]

This branch adds the parse_datetime module from the gnu
portability library. With the following improvements to hwclock:

* Prevent shell injection vulnerabilities from popen() date(1).

* Remove the system dependency on date(1).

* Significant cleanup of hwclock.c code.

* Bug fix for the --predict function

For a parse_datetime2() demonstration this command works well:

./hwclock --predict --date 'next month' -D


Tested with 'make check':
---------------------------------------------------------------------
  All 191 tests PASSED
---------------------------------------------------------------------


The following changes since commit 7ccc1643abdd0cb9780ca0304dfdcbc4c5b11eae:

  findmnt: use line separator for --poll output (2017-02-10 17:28:07 +0100)

are available in the git repository at:

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

for you to fetch changes up to b61e954ec899bd656f7c12d9f93f7f7ef3f0d704:

  hwclock: use gnulib's parse_datetime2 function (2017-02-10 14:55:48 -0500)

----------------------------------------------------------------
J William Piggott (3):
  gnulib: add source for gnulib parse-datetime mod
  build-sys: configure gnulib parse-datetime module
  hwclock: use gnulib's parse_datetime2 function

 Makefile.am                     |   10 +-
 autogen.sh                      |    2 +-
 config/snippet/_Noreturn.h      |   10 +
 config/snippet/arg-nonnull.h    |   26 +
 config/snippet/c++defs.h        |  316 +++++
 config/snippet/warn-on-use.h    |  109 ++
 configure.ac                    |    6 +-
 gllib/Makefile.am               | 1201 +++++++++++++++++++
 gllib/alloca.in.h               |   65 ++
 gllib/basename-lgpl.c           |   75 ++
 gllib/c-ctype.c                 |    3 +
 gllib/c-ctype.h                 |  366 ++++++
 gllib/dirname-lgpl.c            |   86 ++
 gllib/dirname.h                 |   54 +
 gllib/dosname.h                 |   53 +
 gllib/errno.in.h                |  279 +++++
 gllib/error.c                   |  406 +++++++
 gllib/error.h                   |   75 ++
 gllib/exitfail.c                |   24 +
 gllib/exitfail.h                |   18 +
 gllib/flexmember.h              |   42 +
 gllib/getprogname.c             |  185 +++
 gllib/getprogname.h             |   40 +
 gllib/gettext.h                 |  292 +++++
 gllib/gettime.c                 |   48 +
 gllib/gettimeofday.c            |  154 +++
 gllib/gldoc/parse-datetime.texi |  594 ++++++++++
 gllib/intprops.h                |  458 ++++++++
 gllib/limits.in.h               |   63 +
 gllib/malloc.c                  |   56 +
 gllib/malloca.c                 |  149 +++
 gllib/malloca.h                 |  128 ++
 gllib/malloca.valgrind          |    7 +
 gllib/mktime-internal.h         |   37 +
 gllib/mktime.c                  |  630 ++++++++++
 gllib/msvc-inval.c              |  129 +++
 gllib/msvc-inval.h              |  222 ++++
 gllib/msvc-nothrow.c            |   49 +
 gllib/msvc-nothrow.h            |   43 +
 gllib/parse-datetime.h          |   29 +
 gllib/parse-datetime.y          | 2440 +++++++++++++++++++++++++++++++++++++++
 gllib/setenv.c                  |  390 +++++++
 gllib/stdbool.in.h              |  132 +++
 gllib/stddef.in.h               |  110 ++
 gllib/stdint.in.h               |  707 ++++++++++++
 gllib/stdio.in.h                | 1377 ++++++++++++++++++++++
 gllib/stdlib.in.h               |  992 ++++++++++++++++
 gllib/strerror-override.c       |  302 +++++
 gllib/strerror-override.h       |   56 +
 gllib/strerror.c                |   71 ++
 gllib/strftime.c                | 1499 ++++++++++++++++++++++++
 gllib/strftime.h                |   33 +
 gllib/string.in.h               | 1046 +++++++++++++++++
 gllib/stripslash.c              |   45 +
 gllib/sys_time.in.h             |  220 ++++
 gllib/sys_types.in.h            |   53 +
 gllib/time-internal.h           |   49 +
 gllib/time.in.h                 |  297 +++++
 gllib/time_r.c                  |   44 +
 gllib/time_rz.c                 |  322 ++++++
 gllib/timegm.c                  |   40 +
 gllib/timespec.c                |    3 +
 gllib/timespec.h                |  112 ++
 gllib/unistd.c                  |    4 +
 gllib/unistd.in.h               | 1590 +++++++++++++++++++++++++
 gllib/unsetenv.c                |  127 ++
 gllib/verify.h                  |  279 +++++
 gllib/xalloc-die.c              |   41 +
 gllib/xalloc-oversized.h        |   60 +
 gllib/xalloc.h                  |  266 +++++
 gllib/xmalloc.c                 |  122 ++
 glm4/00gnulib.m4                |   46 +
 glm4/absolute-header.m4         |  102 ++
 glm4/alloca.m4                  |  121 ++
 glm4/bison.m4                   |   24 +
 glm4/clock_time.m4              |   31 +
 glm4/dirname.m4                 |   19 +
 glm4/double-slash-root.m4       |   38 +
 glm4/eealloc.m4                 |   31 +
 glm4/environ.m4                 |   47 +
 glm4/errno_h.m4                 |  137 +++
 glm4/error.m4                   |   27 +
 glm4/extensions.m4              |  173 +++
 glm4/extern-inline.m4           |  102 ++
 glm4/flexmember.m4              |   43 +
 glm4/getprogname.m4             |   43 +
 glm4/gettime.m4                 |   13 +
 glm4/gettimeofday.m4            |  138 +++
 glm4/gnulib-cache.m4            |   49 +
 glm4/gnulib-common.m4           |  462 ++++++++
 glm4/gnulib-comp.m4             |  485 ++++++++
 glm4/gnulib-tool.m4             |   57 +
 glm4/include_next.m4            |  223 ++++
 glm4/limits-h.m4                |   31 +
 glm4/longlong.m4                |  113 ++
 glm4/malloc.m4                  |  101 ++
 glm4/malloca.m4                 |   15 +
 glm4/mktime.m4                  |  268 +++++
 glm4/msvc-inval.m4              |   19 +
 glm4/msvc-nothrow.m4            |   10 +
 glm4/multiarch.m4               |   62 +
 glm4/off_t.m4                   |   18 +
 glm4/parse-datetime.m4          |   55 +
 glm4/setenv.m4                  |  160 +++
 glm4/ssize_t.m4                 |   23 +
 glm4/stdbool.m4                 |  108 ++
 glm4/stddef_h.m4                |   51 +
 glm4/stdint.m4                  |  541 +++++++++
 glm4/stdio_h.m4                 |  225 ++++
 glm4/stdlib_h.m4                |  120 ++
 glm4/strerror.m4                |   96 ++
 glm4/strftime.m4                |   28 +
 glm4/string_h.m4                |  120 ++
 glm4/sys_socket_h.m4            |  176 +++
 glm4/sys_time_h.m4              |  111 ++
 glm4/sys_types_h.m4             |   49 +
 glm4/time_h.m4                  |  134 +++
 glm4/time_r.m4                  |   58 +
 glm4/time_rz.m4                 |   21 +
 glm4/timegm.m4                  |   26 +
 glm4/timespec.m4                |   11 +
 glm4/tm_gmtoff.m4               |   14 +
 glm4/unistd_h.m4                |  190 +++
 glm4/warn-on-use.m4             |   47 +
 glm4/wchar_t.m4                 |   24 +
 glm4/wint_t.m4                  |   62 +
 glm4/xalloc.m4                  |    7 +
 sys-utils/Makemodule.am         |    3 +-
 sys-utils/hwclock.c             |  105 +-
 129 files changed, 24876 insertions(+), 105 deletions(-)
 create mode 100644 config/snippet/_Noreturn.h
 create mode 100644 config/snippet/arg-nonnull.h
 create mode 100644 config/snippet/c++defs.h
 create mode 100644 config/snippet/warn-on-use.h
 create mode 100644 gllib/Makefile.am
 create mode 100644 gllib/alloca.in.h
 create mode 100644 gllib/basename-lgpl.c
 create mode 100644 gllib/c-ctype.c
 create mode 100644 gllib/c-ctype.h
 create mode 100644 gllib/dirname-lgpl.c
 create mode 100644 gllib/dirname.h
 create mode 100644 gllib/dosname.h
 create mode 100644 gllib/errno.in.h
 create mode 100644 gllib/error.c
 create mode 100644 gllib/error.h
 create mode 100644 gllib/exitfail.c
 create mode 100644 gllib/exitfail.h
 create mode 100644 gllib/flexmember.h
 create mode 100644 gllib/getprogname.c
 create mode 100644 gllib/getprogname.h
 create mode 100644 gllib/gettext.h
 create mode 100644 gllib/gettime.c
 create mode 100644 gllib/gettimeofday.c
 create mode 100644 gllib/gldoc/parse-datetime.texi
 create mode 100644 gllib/intprops.h
 create mode 100644 gllib/limits.in.h
 create mode 100644 gllib/malloc.c
 create mode 100644 gllib/malloca.c
 create mode 100644 gllib/malloca.h
 create mode 100644 gllib/malloca.valgrind
 create mode 100644 gllib/mktime-internal.h
 create mode 100644 gllib/mktime.c
 create mode 100644 gllib/msvc-inval.c
 create mode 100644 gllib/msvc-inval.h
 create mode 100644 gllib/msvc-nothrow.c
 create mode 100644 gllib/msvc-nothrow.h
 create mode 100644 gllib/parse-datetime.h
 create mode 100644 gllib/parse-datetime.y
 create mode 100644 gllib/setenv.c
 create mode 100644 gllib/stdbool.in.h
 create mode 100644 gllib/stddef.in.h
 create mode 100644 gllib/stdint.in.h
 create mode 100644 gllib/stdio.in.h
 create mode 100644 gllib/stdlib.in.h
 create mode 100644 gllib/strerror-override.c
 create mode 100644 gllib/strerror-override.h
 create mode 100644 gllib/strerror.c
 create mode 100644 gllib/strftime.c
 create mode 100644 gllib/strftime.h
 create mode 100644 gllib/string.in.h
 create mode 100644 gllib/stripslash.c
 create mode 100644 gllib/sys_time.in.h
 create mode 100644 gllib/sys_types.in.h
 create mode 100644 gllib/time-internal.h
 create mode 100644 gllib/time.in.h
 create mode 100644 gllib/time_r.c
 create mode 100644 gllib/time_rz.c
 create mode 100644 gllib/timegm.c
 create mode 100644 gllib/timespec.c
 create mode 100644 gllib/timespec.h
 create mode 100644 gllib/unistd.c
 create mode 100644 gllib/unistd.in.h
 create mode 100644 gllib/unsetenv.c
 create mode 100644 gllib/verify.h
 create mode 100644 gllib/xalloc-die.c
 create mode 100644 gllib/xalloc-oversized.h
 create mode 100644 gllib/xalloc.h
 create mode 100644 gllib/xmalloc.c
 create mode 100644 glm4/00gnulib.m4
 create mode 100644 glm4/absolute-header.m4
 create mode 100644 glm4/alloca.m4
 create mode 100644 glm4/bison.m4
 create mode 100644 glm4/clock_time.m4
 create mode 100644 glm4/dirname.m4
 create mode 100644 glm4/double-slash-root.m4
 create mode 100644 glm4/eealloc.m4
 create mode 100644 glm4/environ.m4
 create mode 100644 glm4/errno_h.m4
 create mode 100644 glm4/error.m4
 create mode 100644 glm4/extensions.m4
 create mode 100644 glm4/extern-inline.m4
 create mode 100644 glm4/flexmember.m4
 create mode 100644 glm4/getprogname.m4
 create mode 100644 glm4/gettime.m4
 create mode 100644 glm4/gettimeofday.m4
 create mode 100644 glm4/gnulib-cache.m4
 create mode 100644 glm4/gnulib-common.m4
 create mode 100644 glm4/gnulib-comp.m4
 create mode 100644 glm4/gnulib-tool.m4
 create mode 100644 glm4/include_next.m4
 create mode 100644 glm4/limits-h.m4
 create mode 100644 glm4/longlong.m4
 create mode 100644 glm4/malloc.m4
 create mode 100644 glm4/malloca.m4
 create mode 100644 glm4/mktime.m4
 create mode 100644 glm4/msvc-inval.m4
 create mode 100644 glm4/msvc-nothrow.m4
 create mode 100644 glm4/multiarch.m4
 create mode 100644 glm4/off_t.m4
 create mode 100644 glm4/parse-datetime.m4
 create mode 100644 glm4/setenv.m4
 create mode 100644 glm4/ssize_t.m4
 create mode 100644 glm4/stdbool.m4
 create mode 100644 glm4/stddef_h.m4
 create mode 100644 glm4/stdint.m4
 create mode 100644 glm4/stdio_h.m4
 create mode 100644 glm4/stdlib_h.m4
 create mode 100644 glm4/strerror.m4
 create mode 100644 glm4/strftime.m4
 create mode 100644 glm4/string_h.m4
 create mode 100644 glm4/sys_socket_h.m4
 create mode 100644 glm4/sys_time_h.m4
 create mode 100644 glm4/sys_types_h.m4
 create mode 100644 glm4/time_h.m4
 create mode 100644 glm4/time_r.m4
 create mode 100644 glm4/time_rz.m4
 create mode 100644 glm4/timegm.m4
 create mode 100644 glm4/timespec.m4
 create mode 100644 glm4/tm_gmtoff.m4
 create mode 100644 glm4/unistd_h.m4
 create mode 100644 glm4/warn-on-use.m4
 create mode 100644 glm4/wchar_t.m4
 create mode 100644 glm4/wint_t.m4
 create mode 100644 glm4/xalloc.m4

[PATCH 1/3] gnulib: add source for gnulib parse-datetime mod

[J William Piggott]

Install gnulib's parse-datetime module (and its dependencies) for use
with hwclock.

Note: the full path to the gnulib source must be used with the gnulib-tool.

Command used:
/path/to/gnulib/src/gnulib-tool --libtool --no-vc-files --source-base=gllib --m4-base=glm4 --doc-base=gllib/gldoc --tests-base=gltests --import parse-datetime

Signed-off-by: J William Piggott <elseifthen@gmx.com>
---

Not reviewable and to large to post.

[PATCH 2/3] build-sys: configure gnulib parse-datetime module

[J William Piggott]

Signed-off-by: J William Piggott <elseifthen@gmx.com>
---
 Makefile.am             | 10 +++++++---
 autogen.sh              |  2 +-
 configure.ac            |  6 +++++-
 sys-utils/Makemodule.am |  3 ++-
 4 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 0bfc27f..f6fe860 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -28,6 +28,9 @@ ul_libfdisk_incdir  = $(top_builddir)/libfdisk/src
 
 ul_libuuid_incdir  = $(top_srcdir)/libuuid/src
 
+gl_libgnu_incdir = $(top_srcdir)/gllib
+gl_libgnu_la = $(top_srcdir)/gllib/libgnu.la
+
 pkgconfigdir = $(usrlib_execdir)/pkgconfig
 
 bashcompletiondir = @bashcompletiondir@
@@ -60,19 +63,20 @@ CHECK_LOCALS =
 EXTRA_DIST =
 CLEANFILES =
 
-SUBDIRS = po
+SUBDIRS = po gllib
 
 RCS_FIND_IGNORE = \( -name SCCS -o -name BitKeeper -o -name .svn -o \
 		-name CVS -o -name .pc -o -name .hg -o -name .git \) -prune -o
 
-ACLOCAL_AMFLAGS = -I m4
+ACLOCAL_AMFLAGS = -I glm4 -I m4
 
 EXTRA_DIST += \
 	.version \
 	autogen.sh \
 	Documentation \
 	po/update-potfiles \
-	README.licensing
+	README.licensing \
+	glm4/gnulib-cache.m4
 
 include tools/Makemodule.am
 include include/Makemodule.am
diff --git a/autogen.sh b/autogen.sh
index 116885b..03ce936 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -116,7 +116,7 @@ if ! grep -q datarootdir po/Makefile.in.in; then
 datadir = @datadir@/g' po/Makefile.in.in
 fi
 $LIBTOOLIZE --force $LT_OPTS
-aclocal -I m4 $AL_OPTS
+aclocal -I glm4 -I m4 $AL_OPTS
 autoconf $AC_OPTS
 autoheader $AH_OPTS
 
diff --git a/configure.ac b/configure.ac
index ad241fe..e69bc77 100644
--- a/configure.ac
+++ b/configure.ac
@@ -7,7 +7,7 @@ AC_INIT([util-linux],
 AC_PREREQ([2.60])
 
 AC_CONFIG_AUX_DIR([config])
-AC_CONFIG_MACRO_DIR([m4])
+AC_CONFIG_MACRO_DIR([glm4])
 dnl AC_USE_SYSTEM_EXTENSIONS must be called before any macros that run
 dnl the compiler (like AC_PROG_LIBTOOL) to avoid autoconf errors.
 AC_USE_SYSTEM_EXTENSIONS
@@ -99,6 +99,7 @@ AC_SUBST([usrlib_execdir])
 AM_PROG_CC_C_O
 AC_PROG_MKDIR_P
 AC_PROG_CC_STDC
+gl_EARLY
 AC_CANONICAL_HOST
 AC_C_CONST
 AC_C_VOLATILE
@@ -195,6 +196,8 @@ AS_IF([test -d "$srcdir/po"], [
   ALL_LINGUAS="af am ar as be bg bn_IN bn ca cs cy da de el en_GB es et eu_ES fa fi fr gl gu he hi hr hu hy id is it ja ka kn ko ku lo lt lv mk ml mr ms my nb nl nn no nso or pa pl pt_BR pt ro ru si sk sl sq sr@Latn sr sv ta te th tr uk ur vi zh_CN zh_TW zu"
 ])
 
+gl_INIT
+
 AC_CHECK_HEADERS([linux/compiler.h linux/blkpg.h linux/major.h], [], [], [
 #ifdef HAVE_LINUX_COMPILER_H
 #include <linux/compiler.h>
@@ -2254,6 +2257,7 @@ libsmartcols/docs/Makefile
 libsmartcols/docs/version.xml
 libsmartcols/src/libsmartcols.h
 po/Makefile.in
+gllib/Makefile
 ])
 
 AC_OUTPUT
diff --git a/sys-utils/Makemodule.am b/sys-utils/Makemodule.am
index 91d3bf0..3e50134 100644
--- a/sys-utils/Makemodule.am
+++ b/sys-utils/Makemodule.am
@@ -437,7 +437,8 @@ hwclock_SOURCES = \
 if LINUX
 hwclock_SOURCES += sys-utils/hwclock-rtc.c
 endif
-hwclock_LDADD = $(LDADD) libcommon.la -lm
+hwclock_LDADD = $(LDADD) $(LIB_CLOCK_GETTIME) $(gl_libgnu_la) libcommon.la -lm
+hwclock_CFLAGS = $(AM_CFLAGS) -I$(gl_libgnu_incdir)
 if HAVE_AUDIT
 hwclock_LDADD += -laudit
 endif

[PATCH 3/3] hwclock: use gnulib's parse_datetime2 function

[J William Piggott]

* hwclock.c: replace interpret_date_string() with parse_datetime2().
   Eliminates shell injection vulnerability from popen() date(1).
   Removes system dependency on date(1).
   Significant cleanup of hwclock.c code.

* Bug fix for "hwclock --predict --date 'bad_date'" printing:
    hwclock: No usable set-to time.  Cannot set clock.

  The message now matches date(1) for an invalid --date argument.

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

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 059ee40..0697533 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -83,6 +83,7 @@
 #include "timeutils.h"
 #include "env.h"
 #include "xalloc.h"
+#include "parse-datetime.h"
 
 #ifdef HAVE_LIBAUDIT
 #include <libaudit.h>
@@ -639,100 +640,6 @@ display_time(const bool hclock_valid, struct timeval hwctime)
 }
 
 /*
- * Interpret the value of the --date option, which is something like
- * "13:05:01". In fact, it can be any of the myriad ASCII strings that
- * specify a time which the "date" program can understand. The date option
- * value in question is our "dateopt" argument.
- *
- * The specified time is in the local time zone.
- *
- * Our output, "*time_p", is a seconds-into-epoch time.
- *
- * We use the "date" program to interpret the date string. "date" must be
- * runnable by issuing the command "date" to the /bin/sh shell. That means
- * in must be in the current PATH.
- *
- * If anything goes wrong (and many things can), we return return code 10
- * and arbitrary *time_p. Otherwise, return code is 0 and *time_p is valid.
- */
-static int interpret_date_string(const struct hwclock_control *ctl,
-				 time_t *const time_p)
-{
-	FILE *date_child_fp = NULL;
-	char *date_command = NULL;
-	char *date_resp = NULL;
-	size_t len = 0;
-	const char magic[] = "seconds-into-epoch=";
-	int retcode = 1;
-	long seconds_since_epoch;
-
-	if (!ctl->date_opt) {
-		warnx(_("No --date option specified."));
-		return retcode;
-	}
-
-	/* Quotes in date_opt would ruin the date command we construct. */
-	if (strchr(ctl->date_opt, '"') != NULL ||
-	    strchr(ctl->date_opt, '`') != NULL ||
-	    strchr(ctl->date_opt, '$') != NULL) {
-		warnx(_
-		      ("The value of the --date option is not a valid date.\n"
-		       "In particular, it contains illegal character(s)."));
-		return retcode;
-	}
-
-	xasprintf(&date_command, "date --date=\"%s\" +%s%%s",
-		ctl->date_opt, magic);
-	if (ctl->debug)
-		printf(_("Issuing date command: %s\n"), date_command);
-
-	date_child_fp = popen(date_command, "r");
-	if (date_child_fp == NULL) {
-		warn(_("Unable to run 'date' program in /bin/sh shell. "
-			    "popen() failed"));
-		goto out;
-	}
-
-	if (getline(&date_resp, &len, date_child_fp) < 0) {
-		warn(_("getline() failed"));
-		goto out;
-	}
-	if (ctl->debug)
-		printf(_("response from date command = %s\n"), date_resp);
-	if (strncmp(date_resp, magic, sizeof(magic) - 1) != 0) {
-		warnx(_("The date command issued by %s returned "
-				  "unexpected results.\n"
-				  "The command was:\n  %s\n"
-				  "The response was:\n  %s"),
-			program_invocation_short_name, date_command, date_resp);
-		goto out;
-	}
-
-	if (sscanf(date_resp + sizeof(magic) - 1, "%ld", &seconds_since_epoch) < 1) {
-		warnx(_("The date command issued by %s returned "
-			"something other than an integer where the "
-			"converted time value was expected.\n"
-			"The command was:\n  %s\n"
-			"The response was:\n %s\n"),
-		      program_invocation_short_name, date_command, date_resp);
-	} else {
-		retcode = 0;
-		*time_p = seconds_since_epoch;
-		if (ctl->debug)
-			printf(_("date string %s equates to "
-				 "%ld seconds since 1969.\n"),
-			       ctl->date_opt, *time_p);
-	}
- out:
-	free(date_command);
-	free(date_resp);
-	if (date_child_fp)
-		pclose(date_child_fp);
-
-	return retcode;
-}
-
-/*
  * Set the System Clock to time 'newtime'.
  *
  * Also set the kernel time zone value to the value indicated by the TZ
@@ -1460,6 +1367,7 @@ int main(int argc, char **argv)
 	struct hwclock_control ctl = { 0 };
 	struct timeval startup_time;
 	struct adjtime adjtime = { 0 };
+	struct timespec when = { 0 };
 	/*
 	 * The time we started up, in seconds into the epoch, including
 	 * fractions.
@@ -1699,11 +1607,10 @@ int main(int argc, char **argv)
 #endif
 
 	if (ctl.set || ctl.predict) {
-		rc = interpret_date_string(&ctl, &set_time);
-		/* (time-consuming) */
-		if (rc != 0) {
-			warnx(_("No usable set-to time.  "
-				"Cannot set clock."));
+		if (parse_datetime2 (&when, ctl.date_opt, NULL, ctl.debug))
+			set_time = when.tv_sec;
+		else {
+			warnx(_("invalid date '%s'"), ctl.date_opt);
 			hwclock_exit(&ctl, EX_USAGE);
 		}
 	}

Re: [PATCH 3/3] hwclock: use gnulib's parse_datetime2 function

[Assaf Gordon]

Hello William,

two minor suggestions:

On Fri, Feb 10, 2017 at 04:08:20PM -0500, J William Piggott wrote:

>+	if (parse_datetime2 (&when, ctl.date_opt, NULL, ctl.debug))

1.
The 'flags' parameter in parse_datetime2 should be
considered as a bit-field, not a boolean.

It's true that in parse-datetime.h [1] the only
existing flag is the debug flag with a value of one,
but we made it so that other future flags are also possible
[2]. I believe 'ctl.debug' is treated like a boolean.

[1] 
http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/parse-datetime.h
[2]
https://lists.gnu.org/archive/html/bug-gnulib/2016-08/msg00024.html


BTW,
Turning on the debug flag for parse_datetime2 will emit
the parsing steps to STDERR, which will look like so:
https://lists.gnu.org/archive/html/bug-gnulib/2016-08/msg00019.html
(might be good for util-linux, or might be an overkill).


2.
The latest version of 'parse_datetime2' function takes 6 parameters,
not just 4. The extra 2 are timezone specific values.
Added here:
http://git.savannah.gnu.org/cgit/gnulib.git/commit/lib?id=4e6e16b3f43ce


regards,
 - assaf

Re: [PATCH 0/3] hwclock: remove date(1)

[Sami Kerola]

On 10 February 2017 at 20:58, J William Piggott <elseifthen@gmx.com> wrote:
> This branch adds the parse_datetime module from the gnu
> portability library.

Assuming gnulib integration is wanted then replacing bootstrap process,
deduplicating pieces of code in include/ with gnulib versions would be in
todo list.  For example why one would keep util-linux xmalloc() when gnulib
provides the same.  Same is true for close_stdout(), and many other
functions.

While code reuse is good idea I have doubts if this really is direction
where to go to.  It feels like gnulib integration is a big change, and easy
only when done incomplete or inadequate way.  Choosing the former is a bit
risky - I assume it is really difficult to know how much coding resource
will stick around doing the needed, how long it might take when reviews and
testing is included to a timeline.

Poor intergration has it's obvious problems.  Duplication of 'same' code,
leading to confusion which code paths are used where.  This can result bugs
that are easy to miss, e.g., end up being part of releases.  If the
integration is kept minimal then we might result in adding 25k lines of code
to replace one exec() that has worked fairly OK for years.  That in inself
feels awkward.

If asked would I be interested of gnulib challenge I would say yes, with a
catch, there is only so few hours per week I have time for this so without a
lot of people saying 'yes, lets do this' timeline getting integration
completed becomes unacceptably long.

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

Re: [PATCH 3/3] hwclock: use gnulib's parse_datetime2 function

[J William Piggott]

On 02/10/2017 04:38 PM, Assaf Gordon wrote:
> Hello William,
> 
> two minor suggestions:
> 
> On Fri, Feb 10, 2017 at 04:08:20PM -0500, J William Piggott wrote:
> 
>> +    if (parse_datetime2 (&when, ctl.date_opt, NULL, ctl.debug))
> 
> 1.  The 'flags' parameter in parse_datetime2 should be considered as a
> bit-field, not a boolean.
>

Hello Assaf,

Thank you, yes I am aware of this. However, I expected push back against
adding gnulib and I didn't want to muddy the discussion waters with this
nuance. So for this simple version it has the effect of turning on
parse_datetime debugging only for odd levels. -D it's on, -DD off, -DDD on ...

*IF* parse_datetime gets accepted, I will address this.
 
> BTW, Turning on the debug flag for parse_datetime2 will emit the
> parsing steps to STDERR, which will look like so:
> https://lists.gnu.org/archive/html/bug-gnulib/2016-08/msg00019.html
> 
> (might be good for util-linux, or might be an overkill).

I think it is useful, but should be at a debug level > 1. Again, *if*
parse_datetime is accepted (which I am not optimistic about after
reading Sami's comments) I will address debugging. I wanted it to be on
at level 1 for this initial introduction so reviewers could observe it
in action. Which is why I offered the suggested testing command in the
cover letter 0/3.

> 
> 2.  The latest version of 'parse_datetime2' function takes 6
> parameters, not just 4. The extra 2 are timezone specific values.
> Added here:
> http://git.savannah.gnu.org/cgit/gnulib.git/commit/lib?id=4e6e16b3f43ce

This I wasn't aware of. Paul added it 2 days after I started this
development branch.

Thank you for the review assaf
> 
> 
> 
> regards,
> - assaf
> 
> 
> -- 
> To
> unsubscribe from
> this list:
> send the
> line
> "unsubscribe
> util-linux" in
> 
> the body
> of a
> message to
> majordomo@vger.kernel.org
> 
> More
> majordomo
> info at 
> http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 0/3] hwclock: remove date(1)

[J William Piggott]

On 02/11/2017 06:24 AM, Sami Kerola wrote:
> On 10 February 2017 at 20:58, J William Piggott <elseifthen@gmx.com> wrote:
>> This branch adds the parse_datetime module from the gnu
>> portability library.
> 
> Assuming gnulib integration is wanted then replacing bootstrap process,
> deduplicating pieces of code in include/ with gnulib versions would be in
> todo list.  For example why one would keep util-linux xmalloc() when gnulib
> provides the same.  Same is true for close_stdout(), and many other
> functions.

Yes, I agree. For example your recent timegm() is covered and I'm sure
many others.

Considering the number of projects using it, and presumably a large base
of contributors, the solutions may be more complete then util-linux's
roll-your-own portability solutions?

There are a number of gnulib modules I see for possible use in hwclock,
like clock-time for example (which is already available to it with this
patch set).

> 
> While code reuse is good idea I have doubts if this really is direction
> where to go to.  It feels like gnulib integration is a big change, and easy
> only when done incomplete or inadequate way.  Choosing the former is a bit
> risky - I assume it is really difficult to know how much coding resource
> will stick around doing the needed, how long it might take when reviews and
> testing is included to a timeline.
> 
> Poor intergration has it's obvious problems.  Duplication of 'same' code,
> leading to confusion which code paths are used where.  This can result bugs
> that are easy to miss, e.g., end up being part of releases. 

It seems to me all of these are good reasons to introduce gnulib slowly
and incrementally.  To expose only a little of the project at a time,
then if something bad happens it's not so catastrophic. As a first step
only adding it only to the hwclock stanza should give a level of
isolation to the rest of the project. Then as time permits it can slowly
be expanded until full integration is achieved.

With each incremental step time can taken to carefully check what should
be replaced and removed.

> If the
> integration is kept minimal then we might result in adding 25k lines of code
> to replace one exec() that has worked fairly OK for years.  That in inself
> feels awkward.

Keeping it minimal could be temporary. The idea would be continue to
implement more of gnulib as time allows.

If we want to settle for 'worked fairly OK' then there's no need to
refactor it. I think I'd rather see --date require a fixed format than
leave this insecure date(1) call there.

> 
> If asked would I be interested of gnulib challenge I would say yes, with a
> catch, there is only so few hours per week I have time for this so without a
> lot of people saying 'yes, lets do this' timeline getting integration
> completed becomes unacceptably long.
>

This is why incremental is the better way to go. If we make it all
or nothing. It will be nothing.

Re: [PATCH 0/3] hwclock: remove date(1)

[Karel Zak]

On Sat, Feb 11, 2017 at 11:24:38AM +0000, Sami Kerola wrote:
> On 10 February 2017 at 20:58, J William Piggott <elseifthen@gmx.com> wrote:
> > This branch adds the parse_datetime module from the gnu
> > portability library.
> 
> Assuming gnulib integration is wanted then replacing bootstrap process,
> deduplicating pieces of code in include/ with gnulib versions would be in
> todo list.  For example why one would keep util-linux xmalloc() when gnulib
> provides the same.  Same is true for close_stdout(), and many other
> functions.
> 
> While code reuse is good idea I have doubts if this really is direction
> where to go to.

I have no doubts, I'm sure that we don't want it. It's overkill.

> to replace one exec() that has worked fairly OK for years.  That in inself
> feels awkward.

Well, from my point of view it would be good enough to use our
parse_timestamp() from lib/timeutils.c to implement 'hwclock --date'
if we would think about it as a new feature. 

Now it's probably too late as I guess date(1) supports more formats,
so we need to keep exec(date) for backward compatibility. I don't see
any significant change that forces us to replace this exec with gnulib.

> If asked would I be interested of gnulib challenge I would say yes, with a
> catch, there is only so few hours per week I have time for this so without a
> lot of people saying 'yes, lets do this' timeline getting integration
> completed becomes unacceptably long.
 
Let's keep util-linux simple and stupid. We don't need portability to
mars rover and obsolete UNIXes like coreutils. Sometimes it's better
to locally duplicate effort (or steal code from another projects)
than ride a gnulib monster.


    Karel

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

Re: [PATCH 0/3] hwclock: remove date(1)

[J William Piggott]

On 02/13/2017 08:04 AM, Karel Zak wrote:
> On Sat, Feb 11, 2017 at 11:24:38AM +0000, Sami Kerola wrote:
>> On 10 February 2017 at 20:58, J William Piggott <elseifthen@gmx.com> wrote:
>>> This branch adds the parse_datetime module from the gnu
>>> portability library.
>>
>> Assuming gnulib integration is wanted then replacing bootstrap process,
>> deduplicating pieces of code in include/ with gnulib versions would be in
>> todo list.  For example why one would keep util-linux xmalloc() when gnulib
>> provides the same.  Same is true for close_stdout(), and many other
>> functions.
>>
>> While code reuse is good idea I have doubts if this really is direction
>> where to go to.
> 
> I have no doubts, I'm sure that we don't want it. It's overkill.
> 
I expected that. That's okay, I learned a lot from making it work.

Would you consider a single file: sys-utils/hwclock-parse-datetime.c?
I don't want to waste time rewriting it if the answer is no.

Re: [PATCH 0/3] hwclock: remove date(1)

[Karel Zak]

On Mon, Feb 13, 2017 at 09:42:23AM -0500, J William Piggott wrote:
> Would you consider a single file: sys-utils/hwclock-parse-datetime.c?
> I don't want to waste time rewriting it if the answer is no.

Would be possible to extend our parse_timestamp() from
lib/timeutils.c? Anyway, sys-utils/hwclock-date.c would be a better
name :-)

    Karel

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