[PATCH 0/8] Pull Request

[PATCH 0/8] Pull Request

[J William Piggott]

The focus of this  patch set is fixing RTC read logic, which is PATCH 4/8.

The following changes since commit 55b3fe7824c261b6b3d2a8869a8cb509fdd2569f:

  setpriv: document accepted formats for naming caps (2017-08-01 11:52:31 +0200)

are available in the git repository at:

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

for you to fetch changes up to fd1fbfc789541957088734df4a30af7b30d36763:

  hwclock: remove busywait tristate return status (2017-08-01 08:41:43 -0400)

----------------------------------------------------------------
J William Piggott (8):
      hwclock: move systz above init clocks read
      hwclock: move rtc permissions test
      hwclock: move drift correction and --predict
      hwclock: fix RTC read logic
      hwclock: correlate hclocktime instead of set_time.
      hwclock: update man page
      hwclock: restore select() timeout warning
      hwclock: remove busywait tristate return status

 sys-utils/hwclock-rtc.c |  16 +++----
 sys-utils/hwclock.8.in  |  26 ++++++++---
 sys-utils/hwclock.c     | 118 ++++++++++++++++++++++++------------------------
 sys-utils/hwclock.h     |   6 ---
 4 files changed, 87 insertions(+), 79 deletions(-)

[PATCH 1/8] hwclock: move systz above init clocks read

[J William Piggott]

The systz option is all about speed, so move it to the
top and simplify the init clocks read test.

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

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index c74e0e2..cec712d 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -1046,8 +1046,11 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 		adjtime->dirty = TRUE;
 	}
 
+	if (ctl->systz)
+		return set_system_clock_timezone(ctl);
+
 	if (ctl->show || ctl->get || ctl->adjust || ctl->hctosys
-	    || (!ctl->noadjfile && !ctl->systz && !ctl->predict)) {
+	    || (!ctl->noadjfile && !ctl->predict)) {
 		/* data from HW-clock are required */
 		rc = synchronize_to_clock_tick(ctl);
 
@@ -1123,8 +1126,6 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 					    hclock_valid, hclocktime);
 	} else if (ctl->hctosys) {
 		return set_system_clock(ctl, hclock_valid, hclocktime);
-	} else if (ctl->systz) {
-		return set_system_clock_timezone(ctl);
 	} else if (ctl->predict) {
 		hclocktime = time_inc(hclocktime, (double)
 				      -(tdrift.tv_sec + tdrift.tv_usec / 1E6));

[PATCH 2/8] hwclock: move rtc permissions test

[J William Piggott]

Move the rtc permissions test below the systz call and
simplify it's 'if' test.

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

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index cec712d..ad9c502 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -1037,9 +1037,6 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 	/* local return code */
 	int rc = 0;
 
-	if (!ctl->systz && !ctl->predict && ur->get_permissions())
-		return EX_NOPERM;
-
 	if ((ctl->set || ctl->systohc || ctl->adjust) &&
 	    (adjtime->local_utc == UTC) != ctl->universal) {
 		adjtime->local_utc = ctl->universal ? UTC : LOCAL;
@@ -1049,6 +1046,9 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 	if (ctl->systz)
 		return set_system_clock_timezone(ctl);
 
+	if (!ctl->predict && ur->get_permissions())
+		return EX_NOPERM;
+
 	if (ctl->show || ctl->get || ctl->adjust || ctl->hctosys
 	    || (!ctl->noadjfile && !ctl->predict)) {
 		/* data from HW-clock are required */

[PATCH 3/8] hwclock: move drift correction and --predict

[J William Piggott]

Move the predict branch above the RTC read branch.

Move drift correction into the RTC read branch, because it
requires an accurate RTC read, and it needs to be skipped for
operations that do not require an RTC read.

Simplify the RTC read branch test.

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

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index ad9c502..1550dfe 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -1042,15 +1042,34 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 		adjtime->local_utc = ctl->universal ? UTC : LOCAL;
 		adjtime->dirty = TRUE;
 	}
+	/*
+	 * Negate the drift correction, because we want to 'predict' a
+	 * Hardware Clock time that includes drift.
+	 */
+	if (ctl->predict) {
+		hclocktime = t2tv(set_time);
+		calculate_adjustment(ctl, adjtime->drift_factor,
+				     adjtime->last_adj_time,
+				     adjtime->not_adjusted,
+				     hclocktime.tv_sec, &tdrift);
+		hclocktime = time_inc(hclocktime, (double)
+				      -(tdrift.tv_sec + tdrift.tv_usec / 1E6));
+		if (ctl->debug) {
+			printf(_ ("Target date:   %ld\n"), set_time);
+			printf(_ ("Predicted RTC: %ld\n"), hclocktime.tv_sec);
+		}
+		display_time(TRUE, hclocktime);
+		return 0;
+	}
 
 	if (ctl->systz)
 		return set_system_clock_timezone(ctl);
 
-	if (!ctl->predict && ur->get_permissions())
+	if (ur->get_permissions())
 		return EX_NOPERM;
 
 	if (ctl->show || ctl->get || ctl->adjust || ctl->hctosys
-	    || (!ctl->noadjfile && !ctl->predict)) {
+	    || !ctl->noadjfile) {
 		/* data from HW-clock are required */
 		rc = synchronize_to_clock_tick(ctl);
 
@@ -1075,22 +1094,17 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 			if (rc && !ctl->set && !ctl->systohc)
 				return EX_IOERR;
 		}
+		/*
+		 * Calculate and apply drift correction to the Hardware Clock
+		 * time for everything except --show
+		 */
+		calculate_adjustment(ctl, adjtime->drift_factor,
+				     adjtime->last_adj_time,
+				     adjtime->not_adjusted,
+				     hclocktime.tv_sec, &tdrift);
+		if (!ctl->show)
+			hclocktime = time_inc(tdrift, hclocktime.tv_sec);
 	}
-	/*
-	 * Calculate Hardware Clock drift for --predict with the user
-	 * supplied --date option time, and with the time read from the
-	 * Hardware Clock for all other operations.  Apply drift correction
-	 * to the Hardware Clock time for everything except --show and
-	 * --predict.  For --predict negate the drift correction, because we
-	 * want to 'predict' a future Hardware Clock time that includes drift.
-	 */
-	hclocktime = ctl->predict ? t2tv(set_time) : hclocktime;
-	calculate_adjustment(ctl, adjtime->drift_factor,
-			     adjtime->last_adj_time,
-			     adjtime->not_adjusted,
-			     hclocktime.tv_sec, &tdrift);
-	if (!ctl->show && !ctl->predict)
-		hclocktime = time_inc(tdrift, hclocktime.tv_sec);
 	if (ctl->show || ctl->get) {
 		display_time(hclock_valid,
 			     time_inc(hclocktime, -time_diff
@@ -1126,15 +1140,6 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 					    hclock_valid, hclocktime);
 	} else if (ctl->hctosys) {
 		return set_system_clock(ctl, hclock_valid, hclocktime);
-	} else if (ctl->predict) {
-		hclocktime = time_inc(hclocktime, (double)
-				      -(tdrift.tv_sec + tdrift.tv_usec / 1E6));
-		if (ctl->debug) {
-			printf(_
-			       ("At %ld seconds after 1969, RTC is predicted to read %ld seconds after 1969.\n"),
-			       set_time, hclocktime.tv_sec);
-		}
-		display_time(TRUE, hclocktime);
 	}
 	if (!ctl->noadjfile)
 		save_adjtime(ctl, adjtime);

[PATCH 4/8] hwclock: fix RTC read logic

[J William Piggott]

Over the past decade a few commits for a corner case problem
have left the RTC read branch in a bad place.

The problem was: when a corrupted RTC could not be read, then
it also could not be reset, because hwclock would error out
due to the RTC read failure.

2.15-rc1 commit 3b96a7a Aug  9 2008
2.19-rc1 commit 5606df5 Dec 29 2010
2.23-rc1 commit ab8f402 Mar 21 2013

The first fix was to ignore a synchronization timeout only for
the busywait branch.

Two and a half years later a commit describing the same problem
took a little more heavy-handed approach by ignoring all
synchronization failures and the RTC read after it, for both of
the RTC set functions.

Because the previous fix also ignored the select() branch timeout
it caused a bogus warning. The chosen workaround for that was to
only print the select() timeout message in debug mode (this is
reverted by another patch).

The problem with these fixes is that we cannot just ignore the
synchronization timeout like that, because then the drift
correction operations will be invalid. The original logic was
correct; we must exit when synchronization fails.

Another problem is that now there are statements between the
timing-critical synchronize-read-timestamp trio (which were
also in the wrong order, but that part of the problem goes
back further in history).

The solution is to skip the RTC read block completely for the
RTC set functions when not also using the --update-drift
option. If we are updating the drift correction factor during
a set function then we must synchronize and read the RTC.
Otherwise reading the RTC is not needed. Anyone trying to set
a corrupt RTC should not be using --update-drift, because the
resulting drift correction factor would be invalid.

Using this approach has the added benefit of significantly
reducing system shutdown time when not using --update-drift:

time ./hwclock --test --systohc; time ./hwclock-master --test --systohc
Test mode: clock was not changed

real    0m0.072s
user    0m0.066s
sys     0m0.003s
Test mode: clock was not changed

real    0m1.000s
user    0m0.169s
sys     0m0.005s

I've see differences as high as 1.518 seconds.

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

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 1550dfe..e172fb1 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -1034,8 +1034,6 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 	struct timeval hclocktime = { 0, 0 };
 	/* Total Hardware Clock drift correction needed. */
 	struct timeval tdrift;
-	/* local return code */
-	int rc = 0;
 
 	if ((ctl->set || ctl->systohc || ctl->adjust) &&
 	    (adjtime->local_utc == UTC) != ctl->universal) {
@@ -1068,32 +1066,26 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 	if (ur->get_permissions())
 		return EX_NOPERM;
 
-	if (ctl->show || ctl->get || ctl->adjust || ctl->hctosys
-	    || !ctl->noadjfile) {
-		/* data from HW-clock are required */
-		rc = synchronize_to_clock_tick(ctl);
-
+	/*
+	 * Read and drift correct RTC time; except for RTC set functions
+	 * without the --update-drift option because: 1) it's not needed;
+	 * 2) it enables setting a corrupted RTC without reading it first;
+	 * 3) it significantly reduces system shutdown time.
+	 */
+	if ( ! ((ctl->set || ctl->systohc) && !ctl->update)) {
 		/*
-		 * We don't error out if the user is attempting to set the
-		 * RTC and synchronization timeout happens - the RTC could
-		 * be functioning but contain invalid time data so we still
-		 * want to allow a user to set the RTC time.
+		 * Timing critical - do not change the order of, or put
+		 * anything between the follow three statements.
+		 * Synchronization failure MUST exit, because all drift
+		 * operations are invalid without it.
 		 */
-		if (rc == RTC_BUSYWAIT_FAILED && !ctl->set && !ctl->systohc)
+		if (synchronize_to_clock_tick(ctl))
 			return EX_IOERR;
+		read_hardware_clock(ctl, &hclock_valid, &hclocktime.tv_sec);
 		gettimeofday(&read_time, NULL);
 
-		/*
-		 * If we can't synchronize to a clock tick,
-		 * we likely can't read from the RTC so
-		 * don't bother reading it again.
-		 */
-		if (!rc) {
-			rc = read_hardware_clock(ctl, &hclock_valid,
-						 &hclocktime.tv_sec);
-			if (rc && !ctl->set && !ctl->systohc)
-				return EX_IOERR;
-		}
+		if (!hclock_valid)
+			return EX_IOERR;
 		/*
 		 * Calculate and apply drift correction to the Hardware Clock
 		 * time for everything except --show

[PATCH 5/8] hwclock: correlate hclocktime instead of set_time.

[J William Piggott]

Correlate hclocktime with set_time instead of the other way around,
because set_time is used for timestamps in the adjtime file so it needs
to be unadulterated.

Also create var startup_hclocktime for correlated time.

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

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index e172fb1..df6b59e 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -1029,9 +1029,14 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 	bool hclock_valid = FALSE;
 	/*
 	 * Tick synchronized time read from the Hardware Clock and
-	 * then drift correct for all operations except --show.
+	 * then drift corrected for all operations except --show.
 	 */
-	struct timeval hclocktime = { 0, 0 };
+	struct timeval hclocktime = { 0 };
+	/*
+	 * hclocktime correlated to startup_time. That is, what drift
+	 * corrected Hardware Clock time would have been at start up.
+	 */
+	struct timeval startup_hclocktime = { 0 };
 	/* Total Hardware Clock drift correction needed. */
 	struct timeval tdrift;
 
@@ -1096,18 +1101,17 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 				     hclocktime.tv_sec, &tdrift);
 		if (!ctl->show)
 			hclocktime = time_inc(tdrift, hclocktime.tv_sec);
+
+		startup_hclocktime =
+		 time_inc(hclocktime, time_diff(startup_time, read_time));
 	}
 	if (ctl->show || ctl->get) {
-		display_time(hclock_valid,
-			     time_inc(hclocktime, -time_diff
-				      (read_time, startup_time)));
+		display_time(hclock_valid, startup_hclocktime);
 	} else if (ctl->set) {
 		set_hardware_clock_exact(ctl, set_time, startup_time);
 		if (!ctl->noadjfile)
-			adjust_drift_factor(ctl, adjtime,
-					    time_inc(t2tv(set_time), time_diff
-						     (read_time, startup_time)),
-					    hclock_valid, hclocktime);
+			adjust_drift_factor(ctl, adjtime, t2tv(set_time),
+					    hclock_valid, startup_hclocktime);
 	} else if (ctl->adjust) {
 		if (tdrift.tv_sec > 0 || tdrift.tv_sec < -1)
 			do_adjustment(ctl, adjtime, hclock_valid,

[PATCH 6/8] hwclock: update man page

[J William Piggott]

Add information about setting the Hardware Clock if it has been
corrupted.

Add information about --update-drift and reduced system shutdown
times for --systohc.

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

diff --git a/sys-utils/hwclock.8.in b/sys-utils/hwclock.8.in
index e0862c9..107e3f1 100644
--- a/sys-utils/hwclock.8.in
+++ b/sys-utils/hwclock.8.in
@@ -4,7 +4,7 @@
 .\"   Authored new section: DATE-TIME CONFIGURATION.
 .\"   Subsections: Keeping Time..., LOCAL vs UTC, POSIX vs 'RIGHT'.
 .\"
-.TH HWCLOCK 8 "April 2015" "util-linux" "System Administration"
+.TH HWCLOCK 8 "July 2017" "util-linux" "System Administration"
 .SH NAME
 hwclock \- time clocks utility
 .SH SYNOPSIS
@@ -197,8 +197,9 @@ Set the Hardware Clock to the time given by the
 option, and update the timestamps in
 .IR @ADJTIME_PATH@ .
 With the
-.B --update-drift
-option (re)calculate the drift factor.
+.B \%\-\-update-drift
+option also (re)calculate the drift factor.  Try it without the option if
+.BR \%\-\-set " fails.  See " \%\-\-update-drift " below."
 .
 .TP
 .B \-\-systz
@@ -234,9 +235,10 @@ changed then a reboot would be required to inform the kernel.
 .BR \-w , \ \-\-systohc
 Set the Hardware Clock from the System Clock, and update the timestamps in
 .IR @ADJTIME_PATH@ .
-When the
-.B --update-drift
-option is given, then also (re)calculate the drift factor.
+With the
+.B \%\-\-update-drift
+option also (re)calculate the drift factor.  Try it without the option if
+.BR \%\-\-systohc " fails.  See " \%\-\-update-drift " below."
 .
 .TP
 .BR \-V , \ \-\-version
@@ -404,6 +406,11 @@ on the drift factor.
 (Re)calculating drift factor on every shutdown delivers suboptimal
 results.  For example, if ephemeral conditions cause the machine to be
 abnormally hot the drift factor calculation would be out of range.
+.IP \(bu 2
+Significantly increased system shutdown times (as of v2.31 when not
+using
+.B \%\-\-update\-drift
+the RTC is not read).
 .PP
 .RB "Having " \%hwclock
 calculate the drift factor is a good starting point, but for optimal
@@ -414,6 +421,13 @@ crafted it should not need to be changed.  Therefore, the old behavior to
 automatically (re)calculate drift was changed and now requires this
 option to be used.  See the discussion below, under
 .BR "The Adjust Function" .
+.PP
+This option requires reading the Hardware Clock before setting it.  If
+it cannot be read, then this option will cause the set functions to fail.
+This can happen, for example, if the Hardware Clock is corrupted by a
+power failure.  In that case, the clock must first be set without this
+option.  Despite it not working, the resulting drift correction factor
+would be invalid anyway.
 .RE
 .
 .SH NOTES

[PATCH 7/8] hwclock: restore select() timeout warning

[J William Piggott]

hwclock now exits on select(2) timeout so restore the warning.

Reverts commit ab8f402952301106ad0bd5c5a51dc8646d1bff64
 and    commit efc4eaf4229f78f14430d8739ddef2c5101f05cc

Except use warnx(), because select() timeout does not set errno.

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

diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
index c738076..a660e32 100644
--- a/sys-utils/hwclock-rtc.c
+++ b/sys-utils/hwclock-rtc.c
@@ -288,9 +288,8 @@ static int synchronize_to_clock_tick_rtc(const struct hwclock_control *ctl)
 			if (0 < rc)
 				ret = 0;
 			else if (rc == 0) {
-				if (ctl->debug)
-					printf(_("select() to %s to wait for clock tick timed out"),
-					       rtc_dev_name);
+				warnx(_("select() to %s to wait for clock tick timed out"),
+				      rtc_dev_name);
 			} else
 				warn(_("select() to %s to wait for clock tick failed"),
 				     rtc_dev_name);

[PATCH 8/8] hwclock: remove busywait tristate return status

[J William Piggott]

The select() synchronization branch only returns success or
fail. There is no reason for the busywait branch to do more.
If synchronization fails for any reason then it must exit,
otherwise all drift correction operation will be invalid.

Signed-off-by: J William Piggott <elseifthen@gmx.com>
---
 sys-utils/hwclock-rtc.c | 11 +++++------
 sys-utils/hwclock.h     |  6 ------
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
index a660e32..c50011a 100644
--- a/sys-utils/hwclock-rtc.c
+++ b/sys-utils/hwclock-rtc.c
@@ -216,9 +216,8 @@ static int busywait_for_rtc_clock_tick(const struct hwclock_control *ctl,
 		       rtc_dev_name);
 	}
 
-	rc = do_rtc_read_ioctl(rtc_fd, &start_time);
-	if (rc)
-		return RTC_BUSYWAIT_FAILED;
+	if (do_rtc_read_ioctl(rtc_fd, &start_time))
+		return 1;
 
 	/*
 	 * Wait for change.  Should be within a second, but in case
@@ -233,13 +232,13 @@ static int busywait_for_rtc_clock_tick(const struct hwclock_control *ctl,
 		gettimeofday(&now, NULL);
 		if (time_diff(now, begin) > 1.5) {
 			warnx(_("Timed out waiting for time change."));
-			return RTC_BUSYWAIT_TIMEOUT;
+			return 1;
 		}
 	} while (1);
 
 	if (rc)
-		return RTC_BUSYWAIT_FAILED;
-	return RTC_BUSYWAIT_OK;
+		return 1;
+	return 0;
 }
 
 /*
diff --git a/sys-utils/hwclock.h b/sys-utils/hwclock.h
index 8843501..6943d8d 100644
--- a/sys-utils/hwclock.h
+++ b/sys-utils/hwclock.h
@@ -9,12 +9,6 @@
 
 #include "c.h"
 
-enum {
-	RTC_BUSYWAIT_OK = 0,
-	RTC_BUSYWAIT_FAILED,
-	RTC_BUSYWAIT_TIMEOUT
-};
-
 struct hwclock_control {
 	char *date_opt;
 	char *adj_file_name;

Re: [PATCH 4/8] hwclock: fix RTC read logic

[J William Piggott]

On 08/01/2017 09:17 AM, J William Piggott wrote:

 8< ----
> 
> 
> Because the previous fix also ignored the select() branch timeout
> it caused a bogus warning. The chosen workaround for that was to
> only print the select() timeout message in debug mode (this is
> reverted by another patch).

 8< ----

I forgot to mention the problem this caused, which is: the other
functions still exit on a select() timeout ... silently!:

$ hwclock --show
$ hwclock --show --debug
select() to /dev/rtc0 to wait for clock tick timed out...synchronization failed

Not good.

Re: [PATCH 4/8] hwclock: fix RTC read logic

[Karel Zak]

On Tue, Aug 01, 2017 at 09:17:08AM -0400, J William Piggott wrote:
> 
> Over the past decade a few commits for a corner case problem
> have left the RTC read branch in a bad place.
> 
> The problem was: when a corrupted RTC could not be read, then
> it also could not be reset, because hwclock would error out
> due to the RTC read failure.
> 
> 2.15-rc1 commit 3b96a7a Aug  9 2008
> 2.19-rc1 commit 5606df5 Dec 29 2010
> 2.23-rc1 commit ab8f402 Mar 21 2013
> 
> The first fix was to ignore a synchronization timeout only for
> the busywait branch.
> 
> Two and a half years later a commit describing the same problem
> took a little more heavy-handed approach by ignoring all
> synchronization failures and the RTC read after it, for both of
> the RTC set functions.
> 
> Because the previous fix also ignored the select() branch timeout
> it caused a bogus warning. The chosen workaround for that was to
> only print the select() timeout message in debug mode (this is
> reverted by another patch).
> 
> The problem with these fixes is that we cannot just ignore the
> synchronization timeout like that, because then the drift
> correction operations will be invalid. The original logic was
> correct; we must exit when synchronization fails.

... but will be possible to set RTC with invalid data?

> Another problem is that now there are statements between the
> timing-critical synchronize-read-timestamp trio (which were
> also in the wrong order, but that part of the problem goes
> back further in history).
> 
> The solution is to skip the RTC read block completely for the
> RTC set functions when not also using the --update-drift
> option. If we are updating the drift correction factor during
> a set function then we must synchronize and read the RTC.
> Otherwise reading the RTC is not needed. Anyone trying to set
> a corrupt RTC should not be using --update-drift, because the
> resulting drift correction factor would be invalid.

OK, I see. The issue is --update-drift.

Maybe it would be nice to be more explicit in the man page and do 
NOT recommend to use --systohc together with --update-drift.

    Karel


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

Re: [PATCH 6/8] hwclock: update man page

[Karel Zak]

On Tue, Aug 01, 2017 at 09:20:59AM -0400, J William Piggott wrote:
> 
> Add information about setting the Hardware Clock if it has been
> corrupted.
> 
> Add information about --update-drift and reduced system shutdown
> times for --systohc.

Ok :-)

    Karel

> 
> Signed-off-by: J William Piggott <elseifthen@gmx.com>
> ---
>  sys-utils/hwclock.8.in | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/sys-utils/hwclock.8.in b/sys-utils/hwclock.8.in
> index e0862c9..107e3f1 100644
> --- a/sys-utils/hwclock.8.in
> +++ b/sys-utils/hwclock.8.in
> @@ -4,7 +4,7 @@
>  .\"   Authored new section: DATE-TIME CONFIGURATION.
>  .\"   Subsections: Keeping Time..., LOCAL vs UTC, POSIX vs 'RIGHT'.
>  .\"
> -.TH HWCLOCK 8 "April 2015" "util-linux" "System Administration"
> +.TH HWCLOCK 8 "July 2017" "util-linux" "System Administration"
>  .SH NAME
>  hwclock \- time clocks utility
>  .SH SYNOPSIS
> @@ -197,8 +197,9 @@ Set the Hardware Clock to the time given by the
>  option, and update the timestamps in
>  .IR @ADJTIME_PATH@ .
>  With the
> -.B --update-drift
> -option (re)calculate the drift factor.
> +.B \%\-\-update-drift
> +option also (re)calculate the drift factor.  Try it without the option if
> +.BR \%\-\-set " fails.  See " \%\-\-update-drift " below."
>  .
>  .TP
>  .B \-\-systz
> @@ -234,9 +235,10 @@ changed then a reboot would be required to inform the kernel.
>  .BR \-w , \ \-\-systohc
>  Set the Hardware Clock from the System Clock, and update the timestamps in
>  .IR @ADJTIME_PATH@ .
> -When the
> -.B --update-drift
> -option is given, then also (re)calculate the drift factor.
> +With the
> +.B \%\-\-update-drift
> +option also (re)calculate the drift factor.  Try it without the option if
> +.BR \%\-\-systohc " fails.  See " \%\-\-update-drift " below."
>  .
>  .TP
>  .BR \-V , \ \-\-version
> @@ -404,6 +406,11 @@ on the drift factor.
>  (Re)calculating drift factor on every shutdown delivers suboptimal
>  results.  For example, if ephemeral conditions cause the machine to be
>  abnormally hot the drift factor calculation would be out of range.
> +.IP \(bu 2
> +Significantly increased system shutdown times (as of v2.31 when not
> +using
> +.B \%\-\-update\-drift
> +the RTC is not read).
>  .PP
>  .RB "Having " \%hwclock
>  calculate the drift factor is a good starting point, but for optimal
> @@ -414,6 +421,13 @@ crafted it should not need to be changed.  Therefore, the old behavior to
>  automatically (re)calculate drift was changed and now requires this
>  option to be used.  See the discussion below, under
>  .BR "The Adjust Function" .
> +.PP
> +This option requires reading the Hardware Clock before setting it.  If
> +it cannot be read, then this option will cause the set functions to fail.
> +This can happen, for example, if the Hardware Clock is corrupted by a
> +power failure.  In that case, the clock must first be set without this
> +option.  Despite it not working, the resulting drift correction factor
> +would be invalid anyway.
>  .RE
>  .
>  .SH NOTES
> --
> 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
> 

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

[PATCH 9/8] hwclock: fix hclock_valid test and error messages

[J William Piggott]

Every hwclock operation that requires reading the RTC, tests
hclock_valid and prints a different warning. This redundancy
is unnecessary.

Move the warning to the RTC read block (the test was moved in
a previous patch in this set). This reduces function arguments
and is a significant code clean up. It will also benefit the
translators.

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

Oops, I forgot this one. It is a complement to 4/8 which moved
the hclock_valid test to the RTC read block. There is no reason
to duplicate these and, as Karel would say, 'be creative' with
each of six warnings. This patch reduces function args, I think
Karel will like that too, because he growls at me when I add
them :). It reduces indention levels and quite a bit of code.

The new results for an invalid RTC return:
$ hwclock
hwclock: RTC read returned an invalid value.

The new results for a failed RTC read:
$ hwclock
hwclock: ioctl(RTC_RD_TIME) to /dev/rtc0 to read the time failed: No such file or directory
hwclock: RTC read returned an invalid value.

I pushed this to the same branch on my repo:

The following changes since commit 87c26ce5b689abe1b52181f98ef3c9eb1b1a5165:

  build-sys: support ncursesw without headers in ncursesw/ directory (2017-08-01 14:36:25 +0200)

are available in the git repository at:

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

for you to fetch changes up to ae4d6cdb30caf61feeb7babb2067dbe4161e48bf:

  hwclock: fix hclock_valid test and error messages (2017-08-02 19:34:58 -0400)

----------------------------------------------------------------
J William Piggott (9):
      hwclock: move systz above init clocks read
      hwclock: move rtc permissions test
      hwclock: move drift correction and --predict
      hwclock: fix RTC read logic
      hwclock: correlate hclocktime instead of set_time.
      hwclock: update man page
      hwclock: restore select() timeout warning
      hwclock: remove busywait tristate return status
      hwclock: fix hclock_valid test and error messages

 sys-utils/hwclock-rtc.c |  16 ++-
 sys-utils/hwclock.8.in  |  31 ++++--
 sys-utils/hwclock.c     | 257 ++++++++++++++++++++----------------------------
 sys-utils/hwclock.h     |   6 --
 4 files changed, 138 insertions(+), 172 deletions(-)

diff --git a/sys-utils/hwclock.8.in b/sys-utils/hwclock.8.in
index 107e3f1..9d475cb 100644
--- a/sys-utils/hwclock.8.in
+++ b/sys-utils/hwclock.8.in
@@ -174,9 +174,8 @@ function.  A third method is to delete the
 will then default to using the UTC timescale for the Hardware Clock.  If
 the Hardware Clock is ticking local time it will need to be defined in
 the file.  This can be done by calling
-.BR hwclock\ \-\-localtime\ \-\-adjust ;
-when the file is not present this command will not actually
-adjust the Clock, but it will create the file with local time
+.BR hwclock\ \-\-localtime\ \-\-systohc ;
+when the file is not present this command will create the file with local time
 configured, and a drift factor of zero.
 .sp
 A condition under which inhibiting
diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index df6b59e..910e39f 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -557,27 +557,15 @@ set_hardware_clock_exact(const struct hwclock_control *ctl,
 	set_hardware_clock(ctl, newhwtime);
 }
 
-/*
- * Put the time "hwctime" on standard output in display format. Except if
- * hclock_valid == false, just tell standard output that we don't know what
- * time it is.
- */
 static void
-display_time(const bool hclock_valid, struct timeval hwctime)
+display_time(struct timeval hwctime)
 {
-	if (!hclock_valid)
-		warnx(_
-		      ("The Hardware Clock registers contain values that are "
-		       "either invalid (e.g. 50th day of month) or beyond the range "
-		       "we can handle (e.g. Year 2095)."));
-	else {
-		char buf[ISO_8601_BUFSIZ];
-
-		strtimeval_iso(&hwctime, ISO_8601_DATE|ISO_8601_TIME|ISO_8601_DOTUSEC|
-					 ISO_8601_TIMEZONE|ISO_8601_SPACE,
-					 buf, sizeof(buf));
-		printf("%s\n", buf);
-	}
+	char buf[ISO_8601_BUFSIZ];
+
+	strtimeval_iso(&hwctime, ISO_8601_DATE|ISO_8601_TIME|ISO_8601_DOTUSEC|
+				 ISO_8601_TIMEZONE|ISO_8601_SPACE,
+				 buf, sizeof(buf));
+	printf("%s\n", buf);
 }
 
 /*
@@ -593,66 +581,55 @@ display_time(const bool hclock_valid, struct timeval hwctime)
  * Clock's timescale configuration is changed then a reboot is required for
  * persistent_clock_is_local to be updated.
  *
- * EXCEPT: if hclock_valid is false, just issue an error message saying
- * there is no valid time in the Hardware Clock to which to set the system
- * time.
- *
  * If 'testing' is true, don't actually update anything -- just say we would
  * have.
  */
 static int
-set_system_clock(const struct hwclock_control *ctl, const bool hclock_valid,
+set_system_clock(const struct hwclock_control *ctl,
 		 const struct timeval newtime)
 {
 	int retcode;
 
-	if (!hclock_valid) {
-		warnx(_
-		      ("The Hardware Clock does not contain a valid time, so "
-		       "we cannot set the System Time from it."));
-		retcode = 1;
-	} else {
-		const struct timeval *tv_null = NULL;
-		struct tm *broken;
-		int minuteswest;
-		int rc = 0;
+	const struct timeval *tv_null = NULL;
+	struct tm *broken;
+	int minuteswest;
+	int rc = 0;
 
-		broken = localtime(&newtime.tv_sec);
+	broken = localtime(&newtime.tv_sec);
 #ifdef HAVE_TM_GMTOFF
-		minuteswest = -broken->tm_gmtoff / 60;	/* GNU extension */
+	minuteswest = -broken->tm_gmtoff / 60;	/* GNU extension */
 #else
-		minuteswest = timezone / 60;
-		if (broken->tm_isdst)
-			minuteswest -= 60;
+	minuteswest = timezone / 60;
+	if (broken->tm_isdst)
+		minuteswest -= 60;
 #endif
 
-		if (ctl->debug) {
-			printf(_("Calling settimeofday:\n"));
-			printf(_("\ttv.tv_sec = %ld, tv.tv_usec = %ld\n"),
-			       newtime.tv_sec, newtime.tv_usec);
-			printf(_("\ttz.tz_minuteswest = %d\n"), minuteswest);
-		}
-		if (ctl->testing) {
-			printf(_
-			       ("Test mode: clock was not changed\n"));
-			retcode = 0;
-		} else {
-			const struct timezone tz = { minuteswest, 0 };
+	if (ctl->debug) {
+		printf(_("Calling settimeofday:\n"));
+		printf(_("\ttv.tv_sec = %ld, tv.tv_usec = %ld\n"),
+		       newtime.tv_sec, newtime.tv_usec);
+		printf(_("\ttz.tz_minuteswest = %d\n"), minuteswest);
+	}
+	if (ctl->testing) {
+		printf(_
+		       ("Test mode: clock was not changed\n"));
+		retcode = 0;
+	} else {
+		const struct timezone tz = { minuteswest, 0 };
 
-			/* Set kernel persistent_clock_is_local so that 11 minute
-			 * mode does not clobber the Hardware Clock with UTC. This
-			 * is only available on first call of settimeofday after boot.
-			 */
-			if (!ctl->universal)
-				rc = settimeofday(tv_null, &tz);
-			if (!rc)
-				rc = settimeofday(&newtime, &tz);
-			if (rc) {
-				warn(_("settimeofday() failed"));
-				retcode = 1;
-			} else
-				retcode = 0;
-		}
+		/* Set kernel persistent_clock_is_local so that 11 minute
+		 * mode does not clobber the Hardware Clock with UTC. This
+		 * is only available on first call of settimeofday after boot.
+		 */
+		if (!ctl->universal)
+			rc = settimeofday(tv_null, &tz);
+		if (!rc)
+			rc = settimeofday(&newtime, &tz);
+		if (rc) {
+			warn(_("settimeofday() failed"));
+			retcode = 1;
+		} else
+			retcode = 0;
 	}
 	return retcode;
 }
@@ -754,27 +731,17 @@ static int set_system_clock_timezone(const struct hwclock_control *ctl)
  * Update the drift factor in <*adjtime_p> based on the fact that the
  * Hardware Clock was just calibrated to <nowtime> and before that was
  * set to the <hclocktime> time scale.
- *
- * EXCEPT: if <hclock_valid> is false, assume Hardware Clock was not set
- * before to anything meaningful and regular adjustments have not been done,
- * so don't adjust the drift factor.
  */
 static void
 adjust_drift_factor(const struct hwclock_control *ctl,
 		    struct adjtime *adjtime_p,
 		    const struct timeval nowtime,
-		    const bool hclock_valid,
 		    const struct timeval hclocktime)
 {
 	if (!ctl->update) {
 		if (ctl->debug)
 			printf(_("Not adjusting drift factor because the "
 				 "--update-drift option was not used.\n"));
-	} else if (!hclock_valid) {
-		if (ctl->debug)
-			printf(_("Not adjusting drift factor because the "
-				 "Hardware Clock previously contained "
-				 "garbage.\n"));
 	} else if (adjtime_p->last_calib_time == 0) {
 		if (ctl->debug)
 			printf(_("Not adjusting drift factor because last "
@@ -936,8 +903,6 @@ static void save_adjtime(const struct hwclock_control *ctl,
  * Do not update anything if the Hardware Clock does not currently present a
  * valid time.
  *
- * <hclock_valid> means the Hardware Clock contains a valid time.
- *
  * <hclocktime> is the drift corrected time read from the Hardware Clock.
  *
  * <read_time> was the system time when the <hclocktime> was read, which due
@@ -954,17 +919,10 @@ static void save_adjtime(const struct hwclock_control *ctl,
  */
 static void
 do_adjustment(const struct hwclock_control *ctl, struct adjtime *adjtime_p,
-	      const bool hclock_valid, const struct timeval hclocktime,
+	      const struct timeval hclocktime,
 	      const struct timeval read_time)
 {
-	if (!hclock_valid) {
-		warnx(_("The Hardware Clock does not contain a valid time, "
-			"so we cannot adjust it."));
-		adjtime_p->last_calib_time = 0;	/* calibration startover is required */
-		adjtime_p->last_adj_time = 0;
-		adjtime_p->not_adjusted = 0;
-		adjtime_p->dirty = TRUE;
-	} else if (adjtime_p->last_adj_time == 0) {
+	if (adjtime_p->last_adj_time == 0) {
 		if (ctl->debug)
 			printf(_("Not setting clock because last adjustment time is zero, "
 				 "so history is bad.\n"));
@@ -1061,7 +1019,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(TRUE, hclocktime);
+		display_time(hclocktime);
 		return 0;
 	}
 
@@ -1089,8 +1047,10 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 		read_hardware_clock(ctl, &hclock_valid, &hclocktime.tv_sec);
 		gettimeofday(&read_time, NULL);
 
-		if (!hclock_valid)
+		if (!hclock_valid) {
+			warnx(_("RTC read returned an invalid value."));
 			return EX_IOERR;
+		}
 		/*
 		 * Calculate and apply drift correction to the Hardware Clock
 		 * time for everything except --show
@@ -1106,16 +1066,15 @@ 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(hclock_valid, startup_hclocktime);
+		display_time(startup_hclocktime);
 	} else if (ctl->set) {
 		set_hardware_clock_exact(ctl, set_time, startup_time);
 		if (!ctl->noadjfile)
 			adjust_drift_factor(ctl, adjtime, t2tv(set_time),
-					    hclock_valid, startup_hclocktime);
+					    startup_hclocktime);
 	} else if (ctl->adjust) {
 		if (tdrift.tv_sec > 0 || tdrift.tv_sec < -1)
-			do_adjustment(ctl, adjtime, hclock_valid,
-				      hclocktime, read_time);
+			do_adjustment(ctl, adjtime, hclocktime, read_time);
 		else
 			printf(_("Needed adjustment is less than one second, "
 				 "so not setting clock.\n"));
@@ -1133,9 +1092,9 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 		set_hardware_clock_exact(ctl, (time_t) reftime.tv_sec, reftime);
 		if (!ctl->noadjfile)
 			adjust_drift_factor(ctl, adjtime, nowtime,
-					    hclock_valid, hclocktime);
+					    hclocktime);
 	} else if (ctl->hctosys) {
-		return set_system_clock(ctl, hclock_valid, hclocktime);
+		return set_system_clock(ctl, hclocktime);
 	}
 	if (!ctl->noadjfile)
 		save_adjtime(ctl, adjtime);

[v2 PATCH 9/8] hwclock: fix hclock_valid test and error messages

[J William Piggott]

Every hwclock operation that requires reading the RTC, tests
hclock_valid and prints a different warning. This redundancy
is unnecessary.

Move the warning to the RTC read block (the test was moved in
a previous patch in this set). This reduces function arguments
and is a significant code clean up. It will also benefit the
translators.

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

This patch does not affect normal operations, so the man page change
was unnecessary. Removed man page change from this patch and pushed it
to the same branch of my repo:

The following changes since commit a6b1ec864a3eb1d27a8781fe99d65e4e6ac05e5b:

  libblkid: add support for UBI superblock (2017-08-03 14:11:21 +0200)

are available in the git repository at:

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

for you to fetch changes up to 88d2a1a312445ba1cb540b960e2a9578b170e820:

  hwclock: fix hclock_valid test and error messages (2017-08-04 08:57:13 -0400)

----------------------------------------------------------------
J William Piggott (9):
      hwclock: move systz above init clocks read
      hwclock: move rtc permissions test
      hwclock: move drift correction and --predict
      hwclock: fix RTC read logic
      hwclock: correlate hclocktime instead of set_time.
      hwclock: update man page
      hwclock: restore select() timeout warning
      hwclock: remove busywait tristate return status
      hwclock: fix hclock_valid test and error messages

 sys-utils/hwclock-rtc.c |  16 ++-
 sys-utils/hwclock.8.in  |  26 +++--
 sys-utils/hwclock.c     | 257 ++++++++++++++++++++----------------------------
 sys-utils/hwclock.h     |   6 --
 4 files changed, 136 insertions(+), 169 deletions(-)

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index df6b59e..910e39f 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -557,27 +557,15 @@ set_hardware_clock_exact(const struct hwclock_control *ctl,
 	set_hardware_clock(ctl, newhwtime);
 }
 
-/*
- * Put the time "hwctime" on standard output in display format. Except if
- * hclock_valid == false, just tell standard output that we don't know what
- * time it is.
- */
 static void
-display_time(const bool hclock_valid, struct timeval hwctime)
+display_time(struct timeval hwctime)
 {
-	if (!hclock_valid)
-		warnx(_
-		      ("The Hardware Clock registers contain values that are "
-		       "either invalid (e.g. 50th day of month) or beyond the range "
-		       "we can handle (e.g. Year 2095)."));
-	else {
-		char buf[ISO_8601_BUFSIZ];
-
-		strtimeval_iso(&hwctime, ISO_8601_DATE|ISO_8601_TIME|ISO_8601_DOTUSEC|
-					 ISO_8601_TIMEZONE|ISO_8601_SPACE,
-					 buf, sizeof(buf));
-		printf("%s\n", buf);
-	}
+	char buf[ISO_8601_BUFSIZ];
+
+	strtimeval_iso(&hwctime, ISO_8601_DATE|ISO_8601_TIME|ISO_8601_DOTUSEC|
+				 ISO_8601_TIMEZONE|ISO_8601_SPACE,
+				 buf, sizeof(buf));
+	printf("%s\n", buf);
 }
 
 /*
@@ -593,66 +581,55 @@ display_time(const bool hclock_valid, struct timeval hwctime)
  * Clock's timescale configuration is changed then a reboot is required for
  * persistent_clock_is_local to be updated.
  *
- * EXCEPT: if hclock_valid is false, just issue an error message saying
- * there is no valid time in the Hardware Clock to which to set the system
- * time.
- *
  * If 'testing' is true, don't actually update anything -- just say we would
  * have.
  */
 static int
-set_system_clock(const struct hwclock_control *ctl, const bool hclock_valid,
+set_system_clock(const struct hwclock_control *ctl,
 		 const struct timeval newtime)
 {
 	int retcode;
 
-	if (!hclock_valid) {
-		warnx(_
-		      ("The Hardware Clock does not contain a valid time, so "
-		       "we cannot set the System Time from it."));
-		retcode = 1;
-	} else {
-		const struct timeval *tv_null = NULL;
-		struct tm *broken;
-		int minuteswest;
-		int rc = 0;
+	const struct timeval *tv_null = NULL;
+	struct tm *broken;
+	int minuteswest;
+	int rc = 0;
 
-		broken = localtime(&newtime.tv_sec);
+	broken = localtime(&newtime.tv_sec);
 #ifdef HAVE_TM_GMTOFF
-		minuteswest = -broken->tm_gmtoff / 60;	/* GNU extension */
+	minuteswest = -broken->tm_gmtoff / 60;	/* GNU extension */
 #else
-		minuteswest = timezone / 60;
-		if (broken->tm_isdst)
-			minuteswest -= 60;
+	minuteswest = timezone / 60;
+	if (broken->tm_isdst)
+		minuteswest -= 60;
 #endif
 
-		if (ctl->debug) {
-			printf(_("Calling settimeofday:\n"));
-			printf(_("\ttv.tv_sec = %ld, tv.tv_usec = %ld\n"),
-			       newtime.tv_sec, newtime.tv_usec);
-			printf(_("\ttz.tz_minuteswest = %d\n"), minuteswest);
-		}
-		if (ctl->testing) {
-			printf(_
-			       ("Test mode: clock was not changed\n"));
-			retcode = 0;
-		} else {
-			const struct timezone tz = { minuteswest, 0 };
+	if (ctl->debug) {
+		printf(_("Calling settimeofday:\n"));
+		printf(_("\ttv.tv_sec = %ld, tv.tv_usec = %ld\n"),
+		       newtime.tv_sec, newtime.tv_usec);
+		printf(_("\ttz.tz_minuteswest = %d\n"), minuteswest);
+	}
+	if (ctl->testing) {
+		printf(_
+		       ("Test mode: clock was not changed\n"));
+		retcode = 0;
+	} else {
+		const struct timezone tz = { minuteswest, 0 };
 
-			/* Set kernel persistent_clock_is_local so that 11 minute
-			 * mode does not clobber the Hardware Clock with UTC. This
-			 * is only available on first call of settimeofday after boot.
-			 */
-			if (!ctl->universal)
-				rc = settimeofday(tv_null, &tz);
-			if (!rc)
-				rc = settimeofday(&newtime, &tz);
-			if (rc) {
-				warn(_("settimeofday() failed"));
-				retcode = 1;
-			} else
-				retcode = 0;
-		}
+		/* Set kernel persistent_clock_is_local so that 11 minute
+		 * mode does not clobber the Hardware Clock with UTC. This
+		 * is only available on first call of settimeofday after boot.
+		 */
+		if (!ctl->universal)
+			rc = settimeofday(tv_null, &tz);
+		if (!rc)
+			rc = settimeofday(&newtime, &tz);
+		if (rc) {
+			warn(_("settimeofday() failed"));
+			retcode = 1;
+		} else
+			retcode = 0;
 	}
 	return retcode;
 }
@@ -754,27 +731,17 @@ static int set_system_clock_timezone(const struct hwclock_control *ctl)
  * Update the drift factor in <*adjtime_p> based on the fact that the
  * Hardware Clock was just calibrated to <nowtime> and before that was
  * set to the <hclocktime> time scale.
- *
- * EXCEPT: if <hclock_valid> is false, assume Hardware Clock was not set
- * before to anything meaningful and regular adjustments have not been done,
- * so don't adjust the drift factor.
  */
 static void
 adjust_drift_factor(const struct hwclock_control *ctl,
 		    struct adjtime *adjtime_p,
 		    const struct timeval nowtime,
-		    const bool hclock_valid,
 		    const struct timeval hclocktime)
 {
 	if (!ctl->update) {
 		if (ctl->debug)
 			printf(_("Not adjusting drift factor because the "
 				 "--update-drift option was not used.\n"));
-	} else if (!hclock_valid) {
-		if (ctl->debug)
-			printf(_("Not adjusting drift factor because the "
-				 "Hardware Clock previously contained "
-				 "garbage.\n"));
 	} else if (adjtime_p->last_calib_time == 0) {
 		if (ctl->debug)
 			printf(_("Not adjusting drift factor because last "
@@ -936,8 +903,6 @@ static void save_adjtime(const struct hwclock_control *ctl,
  * Do not update anything if the Hardware Clock does not currently present a
  * valid time.
  *
- * <hclock_valid> means the Hardware Clock contains a valid time.
- *
  * <hclocktime> is the drift corrected time read from the Hardware Clock.
  *
  * <read_time> was the system time when the <hclocktime> was read, which due
@@ -954,17 +919,10 @@ static void save_adjtime(const struct hwclock_control *ctl,
  */
 static void
 do_adjustment(const struct hwclock_control *ctl, struct adjtime *adjtime_p,
-	      const bool hclock_valid, const struct timeval hclocktime,
+	      const struct timeval hclocktime,
 	      const struct timeval read_time)
 {
-	if (!hclock_valid) {
-		warnx(_("The Hardware Clock does not contain a valid time, "
-			"so we cannot adjust it."));
-		adjtime_p->last_calib_time = 0;	/* calibration startover is required */
-		adjtime_p->last_adj_time = 0;
-		adjtime_p->not_adjusted = 0;
-		adjtime_p->dirty = TRUE;
-	} else if (adjtime_p->last_adj_time == 0) {
+	if (adjtime_p->last_adj_time == 0) {
 		if (ctl->debug)
 			printf(_("Not setting clock because last adjustment time is zero, "
 				 "so history is bad.\n"));
@@ -1061,7 +1019,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(TRUE, hclocktime);
+		display_time(hclocktime);
 		return 0;
 	}
 
@@ -1089,8 +1047,10 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 		read_hardware_clock(ctl, &hclock_valid, &hclocktime.tv_sec);
 		gettimeofday(&read_time, NULL);
 
-		if (!hclock_valid)
+		if (!hclock_valid) {
+			warnx(_("RTC read returned an invalid value."));
 			return EX_IOERR;
+		}
 		/*
 		 * Calculate and apply drift correction to the Hardware Clock
 		 * time for everything except --show
@@ -1106,16 +1066,15 @@ 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(hclock_valid, startup_hclocktime);
+		display_time(startup_hclocktime);
 	} else if (ctl->set) {
 		set_hardware_clock_exact(ctl, set_time, startup_time);
 		if (!ctl->noadjfile)
 			adjust_drift_factor(ctl, adjtime, t2tv(set_time),
-					    hclock_valid, startup_hclocktime);
+					    startup_hclocktime);
 	} else if (ctl->adjust) {
 		if (tdrift.tv_sec > 0 || tdrift.tv_sec < -1)
-			do_adjustment(ctl, adjtime, hclock_valid,
-				      hclocktime, read_time);
+			do_adjustment(ctl, adjtime, hclocktime, read_time);
 		else
 			printf(_("Needed adjustment is less than one second, "
 				 "so not setting clock.\n"));
@@ -1133,9 +1092,9 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 		set_hardware_clock_exact(ctl, (time_t) reftime.tv_sec, reftime);
 		if (!ctl->noadjfile)
 			adjust_drift_factor(ctl, adjtime, nowtime,
-					    hclock_valid, hclocktime);
+					    hclocktime);
 	} else if (ctl->hctosys) {
-		return set_system_clock(ctl, hclock_valid, hclocktime);
+		return set_system_clock(ctl, hclocktime);
 	}
 	if (!ctl->noadjfile)
 		save_adjtime(ctl, adjtime);

Re: [PATCH 0/8] Pull Request

[Karel Zak]

On Tue, Aug 01, 2017 at 09:10:14AM -0400, J William Piggott wrote:
>       hwclock: move systz above init clocks read
>       hwclock: move rtc permissions test
>       hwclock: move drift correction and --predict
>       hwclock: fix RTC read logic
>       hwclock: correlate hclocktime instead of set_time.
>       hwclock: update man page
>       hwclock: restore select() timeout warning
>       hwclock: remove busywait tristate return status

Applied (merged from github), thanks!

    Karel

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