[PATCH 0/7] hwclock patch cover letter.

[PATCH 0/7] hwclock patch cover letter.

[JWP]

This hwclock patch set includes three topics, one
is needed functionality, the other two are bug
fixes.

The first also fixes three bugs as a side effect
(See 'Part II' in its commit message).

The second fixes a complaint we receive regularly
from users that keep their Hardware Clock set to
the local timescale while running NTP.

Note: to test this fix you will need a kernel >= 3.13,
as 11 minute mode was broken from 3.0 ~ 3.13-rc1.

The third involves the use of --systohc at
shutdown. We need this with 11 minute mode to
refresh the adjtime file's timestamps, but doing
so clobbers the drift factor to near zero.  When
not running NTP it causes drift factor creep by
assuming that the System Clock is keeping perfect
time. Of course, it is not.

J William Piggott (7):
  hwclock: hctosys drift compensation II
  hwclock: hctosys drift compensation II COMMENTS
  hwclock: hctosys drift compensation II MAN
  hwclock: persistent_clock_is_local
  hwclock: persistent_clock_is_local MAN
  hwclock: Add --update option
  hwclock: Add --update option MAN


 sys-utils/hwclock.8.in | 145 +++++++++++++++------
 sys-utils/hwclock.c    | 337 ++++++++++++++++++++++++++-----------------------
 2 files changed, 281 insertions(+), 201 deletions(-)

[PATCH 1/7] hwclock: hctosys drift compensation II

[JWP]

Allowing hctosys to drift compensate facilitates:

More precise setting of the System Clock early in
the boot process when --adjust cannot be used
because the file system is not writeable.

Applies sub second drift corrections immediately,
where as --adjust cannot.

Reduces boot time by not calling hwclock multiple
times, e.g., --hctosys early before fsck when the
file system is read-only, then --adjust later when
the file system is read-write and --hctosys again
for drift correction.

Use of --adjust elsewhere may no longer be
necessary.

Part II

After the original submission of this patch I
realized that now all operations except --systz
require drift corrected Hardware Clock time.
Therefore, it should be done only once early in
the process. Upon implementation of that premise
many improvements were facilitated:

* Adds drift correction to --hctosys.
* Adds setting system time with sub-second precision.
* Adds --get, a drift corrected 'show' operation.
* Improves drift factor calculation precision while
   greatly simplifying its algorithm.
* Fixes --show bug, printing integer sub-seconds, and
   now uses a more intuitive positive value.
* Fixes --predict bug, drift correction must be
   negated to predict future RTC time.
* Reduces the number of function arguments and
   lines of code.

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

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 474e04f..0dd9ad6 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -181,6 +181,18 @@ static void read_date_from_file(struct tm *tm)
 }
 
 /*
+ * Type time_t to type timeval conversion.
+ */
+static struct timeval t2tv(time_t timet)
+{
+  struct timeval rettimeval;
+
+  rettimeval.tv_sec = timet;
+  rettimeval.tv_usec = 0;
+  return(rettimeval);
+}
+
+/*
  * The difference in seconds between two times in "timeval" format.
  */
 double time_diff(struct timeval subtrahend, struct timeval subtractor)
@@ -678,8 +690,7 @@ set_hardware_clock_exact(const time_t sethwtime,
  * Include in the output the adjustment "sync_duration".
  */
 static void
-display_time(const bool hclock_valid, const time_t systime,
-	     const double sync_duration)
+display_time(const bool hclock_valid, struct timeval hwctime)
 {
 	if (!hclock_valid)
 		warnx(_
@@ -691,9 +702,9 @@ display_time(const bool hclock_valid, const time_t systime,
 		char *format = "%c";
 		char ctime_now[200];
 
-		lt = localtime(&systime);
+		lt = localtime(&hwctime.tv_sec);
 		strftime(ctime_now, sizeof(ctime_now), format, lt);
-		printf(_("%s  %.6f seconds\n"), ctime_now, -(sync_duration));
+		printf(_("%s  .%06d seconds\n"), ctime_now, (int)hwctime.tv_usec);
 	}
 }
 
@@ -807,7 +818,7 @@ static int interpret_date_string(const char *date_opt, time_t * const time_p)
  * have.
  */
 static int
-set_system_clock(const bool hclock_valid, const time_t newtime,
+set_system_clock(const bool hclock_valid, const struct timeval newtime,
 		 const bool testing)
 {
 	int retcode;
@@ -818,15 +829,11 @@ set_system_clock(const bool hclock_valid, const time_t newtime,
 		       "we cannot set the System Time from it."));
 		retcode = 1;
 	} else {
-		struct timeval tv;
 		struct tm *broken;
 		int minuteswest;
 		int rc;
 
-		tv.tv_sec = newtime;
-		tv.tv_usec = 0;
-
-		broken = localtime(&newtime);
+		broken = localtime(&newtime.tv_sec);
 #ifdef HAVE_TM_GMTOFF
 		minuteswest = -broken->tm_gmtoff / 60;	/* GNU extension */
 #else
@@ -838,7 +845,7 @@ set_system_clock(const bool hclock_valid, const time_t newtime,
 		if (debug) {
 			printf(_("Calling settimeofday:\n"));
 			printf(_("\ttv.tv_sec = %ld, tv.tv_usec = %ld\n"),
-			       (long)tv.tv_sec, (long)tv.tv_usec);
+			       (long)newtime.tv_sec, (long)newtime.tv_usec);
 			printf(_("\ttz.tz_minuteswest = %d\n"), minuteswest);
 		}
 		if (testing) {
@@ -848,7 +855,7 @@ set_system_clock(const bool hclock_valid, const time_t newtime,
 		} else {
 			const struct timezone tz = { minuteswest, 0 };
 
-			rc = settimeofday(&tv, &tz);
+			rc = settimeofday(&newtime, &tz);
 			if (rc) {
 				if (errno == EPERM) {
 					warnx(_
@@ -974,9 +981,9 @@ static int set_system_clock_timezone(const bool universal, const bool testing)
  */
 static void
 adjust_drift_factor(struct adjtime *adjtime_p,
-		    const time_t nowtime,
+		    const struct timeval nowtime,
 		    const bool hclock_valid,
-		    const time_t hclocktime, const double sync_delay)
+		    const struct timeval hclocktime)
 {
 	if (!hclock_valid) {
 		if (debug)
@@ -989,7 +996,7 @@ adjust_drift_factor(struct adjtime *adjtime_p,
 				 "calibration time is zero,\n"
 				 "so history is bad and calibration startover "
 				 "is necessary.\n"));
-	} else if ((hclocktime - adjtime_p->last_calib_time) < 24 * 60 * 60) {
+	} else if ((hclocktime.tv_sec - adjtime_p->last_calib_time) < 24 * 60 * 60) {
 		if (debug)
 			printf(_("Not adjusting drift factor because it has "
 				 "been less than a day since the last "
@@ -1009,39 +1016,16 @@ adjust_drift_factor(struct adjtime *adjtime_p,
 		 * but 195 days + 1 second equals 195 days in floats.)
 		 */
 		const double sec_per_day = 24.0 * 60.0 * 60.0;
-		double atime_per_htime;
-		double adj_days, cal_days;
-		double exp_drift, unc_drift;
 		double factor_adjust;
 		double drift_factor;
+	        struct timeval lastCalib;
 
-		/* Adjusted time units per hardware time unit */
-		atime_per_htime = 1.0 + adjtime_p->drift_factor / sec_per_day;
+		lastCalib = t2tv(adjtime_p->last_calib_time);
 
-		/* Days since last adjustment (in hardware clock time) */
-		adj_days = (double)(hclocktime - adjtime_p->last_adj_time)
-		    / sec_per_day;
+		factor_adjust = time_diff(nowtime, hclocktime) /
+				(time_diff(nowtime, lastCalib) / sec_per_day);
 
-		/* Expected drift (sec) since last adjustment */
-		exp_drift = adj_days * adjtime_p->drift_factor
-		    + adjtime_p->not_adjusted;
-
-		/* Uncorrected drift (sec) since last calibration */
-		unc_drift = (double)(nowtime - hclocktime)
-		    + sync_delay - exp_drift;
-
-		/* Days since last calibration (in hardware clock time) */
-		cal_days = ((double)(adjtime_p->last_adj_time
-				     - adjtime_p->last_calib_time)
-			    + adjtime_p->not_adjusted)
-		    / (sec_per_day * atime_per_htime) + adj_days;
-
-		/* Amount to add to previous drift factor */
-		factor_adjust = unc_drift / cal_days;
-
-		/* New drift factor */
 		drift_factor = adjtime_p->drift_factor + factor_adjust;
-
 		if (abs(drift_factor) > MAX_DRIFT) {
 			if (debug)
 				printf(_("Clock drift factor was calculated as "
@@ -1052,19 +1036,19 @@ adjust_drift_factor(struct adjtime *adjtime_p,
 		} else {
 			if (debug)
 				printf(_("Clock drifted %.1f seconds in the past "
-					 "%d seconds in spite of a drift factor of "
+					 "%.1f seconds\nin spite of a drift factor of "
 					 "%f seconds/day.\n"
 					 "Adjusting drift factor by %f seconds/day\n"),
-				       unc_drift,
-				       (int)(nowtime - adjtime_p->last_calib_time),
+				       time_diff(nowtime, hclocktime),
+				       time_diff(nowtime, lastCalib),
 				       adjtime_p->drift_factor, factor_adjust);
 		}
 
 		adjtime_p->drift_factor = drift_factor;
 	}
-	adjtime_p->last_calib_time = nowtime;
+	adjtime_p->last_calib_time = nowtime.tv_sec;
 
-	adjtime_p->last_adj_time = nowtime;
+	adjtime_p->last_adj_time = nowtime.tv_sec;
 
 	adjtime_p->not_adjusted = 0;
 
@@ -1087,26 +1071,23 @@ static void
 calculate_adjustment(const double factor,
 		     const time_t last_time,
 		     const double not_adjusted,
-		     const time_t systime, int *adjustment_p, double *retro_p)
+		     const time_t systime, struct timeval *tdrift_p)
 {
 	double exact_adjustment;
 
 	exact_adjustment =
 	    ((double)(systime - last_time)) * factor / (24 * 60 * 60)
 	    + not_adjusted;
-	*adjustment_p = FLOOR(exact_adjustment);
-
-	*retro_p = exact_adjustment - (double)*adjustment_p;
+	tdrift_p->tv_sec = FLOOR(exact_adjustment);
+	tdrift_p->tv_usec = (exact_adjustment -
+				 (double)tdrift_p->tv_sec) * 1E6;
 	if (debug) {
 		printf(P_("Time since last adjustment is %d second\n",
 			"Time since last adjustment is %d seconds\n",
 		       (int)(systime - last_time)),
 		       (int)(systime - last_time));
-		printf(P_("Need to insert %d second and refer time back "
-			 "%.6f seconds ago\n",
-			 "Need to insert %d seconds and refer time back "
-			 "%.6f seconds ago\n", *adjustment_p),
-			 *adjustment_p, *retro_p);
+		printf(_("Calculated Hardware Clock drift is %ld.%06d seconds\n"),
+		       (long)tdrift_p->tv_sec, (int)tdrift_p->tv_usec);
 	}
 }
 
@@ -1200,7 +1181,7 @@ static void save_adjtime(const struct adjtime adjtime, const bool testing)
  */
 static void
 do_adjustment(struct adjtime *adjtime_p,
-	      const bool hclock_valid, const time_t hclocktime,
+	      const bool hclock_valid, const struct timeval hclocktime,
 	      const struct timeval read_time,
 	      const bool universal, const bool testing)
 {
@@ -1220,27 +1201,13 @@ do_adjustment(struct adjtime *adjtime_p,
 			printf(_("Not setting clock because drift factor %f is far too high.\n"),
 				adjtime_p->drift_factor);
 	} else {
-		int adjustment;
-		/* Number of seconds we must insert in the Hardware Clock */
-		double retro;
-		/*
-		 * Fraction of second we have to remove from clock after
-		 * inserting <adjustment> whole seconds.
-		 */
-		calculate_adjustment(adjtime_p->drift_factor,
-				     adjtime_p->last_adj_time,
-				     adjtime_p->not_adjusted,
-				     hclocktime, &adjustment, &retro);
-		if (adjustment > 0 || adjustment < -1) {
-			set_hardware_clock_exact(hclocktime + adjustment,
-						 time_inc(read_time, -retro),
-						 universal, testing);
-			adjtime_p->last_adj_time = hclocktime + adjustment;
-			adjtime_p->not_adjusted = 0;
-			adjtime_p->dirty = TRUE;
-		} else if (debug)
-			printf(_("Needed adjustment is less than one second, "
-				 "so not setting clock.\n"));
+		set_hardware_clock_exact(hclocktime.tv_sec,
+					 time_inc(read_time,
+						  -(hclocktime.tv_usec / 1E6)),
+					 universal, testing);
+		adjtime_p->last_adj_time = hclocktime.tv_sec;
+		adjtime_p->not_adjusted = 0;
+		adjtime_p->dirty = TRUE;
 	}
 }
 
@@ -1283,7 +1250,7 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
 		 const bool hctosys, const bool systohc, const bool systz,
 		 const struct timeval startup_time,
 		 const bool utc, const bool local_opt,
-		 const bool testing, const bool predict)
+		 const bool testing, const bool predict, const bool get)
 {
 	/* Contents of the adjtime file, or what they should be. */
 	struct adjtime adjtime;
@@ -1302,7 +1269,8 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
 	 * synchronized to its next clock tick when we
 	 * started up. Defined only if hclock_valid is true.
 	 */
-	time_t hclocktime = 0;
+	struct timeval hclocktime = { 0, 0 };
+	struct timeval tdrift;
 	/* local return code */
 	int rc = 0;
 
@@ -1312,13 +1280,12 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
 			return EX_NOPERM;
 	}
 
-	if (!noadjfile
-	    && (adjust || set || systohc || (!utc && !local_opt) || predict)) {
+	if (!noadjfile && !(systz && (utc || local_opt))) {
 		rc = read_adjtime(&adjtime);
 		if (rc)
 			return rc;
 	} else {
-		/* A little trick to avoid reading the file if we don't have to */
+		/* A little trick to avoid writing the file if we don't have to */
 		adjtime.dirty = FALSE;
 	}
 
@@ -1330,7 +1297,7 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
 		adjtime.dirty = TRUE;
 	}
 
-	if (show || adjust || hctosys || (!noadjfile && !systz && !predict)) {
+	if (show || get || adjust || hctosys || (!noadjfile && !systz && !predict)) {
 		/* data from HW-clock are required */
 		rc = synchronize_to_clock_tick();
 
@@ -1353,26 +1320,40 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
 		 */
 		if (!rc) {
 			rc = read_hardware_clock(universal,
-						 &hclock_valid, &hclocktime);
+						 &hclock_valid, &hclocktime.tv_sec);
 			if (rc && !set && !systohc)
 				return EX_IOERR;
 		}
 	}
-
-	if (show) {
-		display_time(hclock_valid, hclocktime,
-			     time_diff(read_time, startup_time));
+	hclocktime = predict ? t2tv(set_time) : hclocktime;
+	calculate_adjustment(adjtime.drift_factor,
+			     adjtime.last_adj_time,
+			     adjtime.not_adjusted,
+			     hclocktime.tv_sec, &tdrift);
+	if (!show && !predict)
+		hclocktime = time_inc(tdrift, hclocktime.tv_sec);
+	if (predict)
+		hclocktime = time_inc(hclocktime, (double)
+				      -(tdrift.tv_sec + tdrift.tv_sec / 1E6));
+	if (show || get) {
+		display_time(hclock_valid,
+			     time_inc(hclocktime, -time_diff
+				      (read_time, startup_time)));
 	} else if (set) {
 		set_hardware_clock_exact(set_time, startup_time,
 					 universal, testing);
 		if (!noadjfile)
-			adjust_drift_factor(&adjtime, set_time,
-					    hclock_valid,
-					    hclocktime,
-					    time_diff(read_time, startup_time));
+			adjust_drift_factor(&adjtime,
+					    time_inc(t2tv(set_time), time_diff
+						     (read_time, startup_time)),
+					    hclock_valid, hclocktime);
 	} else if (adjust) {
-		do_adjustment(&adjtime, hclock_valid,
-			      hclocktime, read_time, universal, testing);
+		if (tdrift.tv_sec > 0 || tdrift.tv_sec < -1)
+			do_adjustment(&adjtime, hclock_valid,
+				      hclocktime, read_time, universal, testing);
+		else
+			printf(_("Needed adjustment is less than one second, "
+				 "so not setting clock.\n"));
 	} else if (systohc) {
 		struct timeval nowtime, reftime;
 		/*
@@ -1388,10 +1369,8 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
 					 reftime.tv_sec,
 					 reftime, universal, testing);
 		if (!noadjfile)
-			adjust_drift_factor(&adjtime, (time_t)
-					    reftime.tv_sec,
-					    hclock_valid, hclocktime, (double)
-					    read_time.tv_usec / 1E6);
+			adjust_drift_factor(&adjtime, nowtime,
+					    hclock_valid, hclocktime);
 	} else if (hctosys) {
 		rc = set_system_clock(hclock_valid, hclocktime, testing);
 		if (rc) {
@@ -1405,19 +1384,12 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
 			return rc;
 		}
 	} else if (predict) {
-		int adjustment;
-		double retro;
-
-		calculate_adjustment(adjtime.drift_factor,
-				     adjtime.last_adj_time,
-				     adjtime.not_adjusted,
-				     set_time, &adjustment, &retro);
 		if (debug) {
 			printf(_
 			       ("At %ld seconds after 1969, RTC is predicted to read %ld seconds after 1969.\n"),
-			       set_time, set_time + adjustment);
+			       set_time, (long)hclocktime.tv_sec);
 		}
-		display_time(TRUE, set_time + adjustment, -retro);
+		display_time(TRUE, hclocktime);
 	}
 	if (!noadjfile)
 		save_adjtime(adjtime, testing);
@@ -1646,7 +1618,7 @@ int main(int argc, char **argv)
 	/* Variables set by various options; show may also be set later */
 	/* The options debug, badyear and epoch_option are global */
 	bool show, set, systohc, hctosys, systz, adjust, getepoch, setepoch,
-	    predict, compare;
+	    predict, compare, get;
 	bool utc, testing, local_opt, noadjfile, directisa;
 	char *date_opt;
 #ifdef __alpha__
@@ -1659,6 +1631,7 @@ int main(int argc, char **argv)
 		OPT_DATE,
 		OPT_DIRECTISA,
 		OPT_EPOCH,
+		OPT_GET,
 		OPT_GETEPOCH,
 		OPT_LOCALTIME,
 		OPT_NOADJFILE,
@@ -1706,13 +1679,14 @@ int main(int argc, char **argv)
 		{"adjfile",	1, 0, OPT_ADJFILE},
 		{"systz",	0, 0, OPT_SYSTZ},
 		{"predict-hc",	0, 0, OPT_PREDICT_HC},
+		{"get",		0, 0, OPT_GET},
 		{NULL,		0, NULL, 0}
 	};
 
 	static const ul_excl_t excl[] = {	/* rows and cols in in ASCII order */
 		{ 'a','r','s','w',
-		  OPT_GETEPOCH,	OPT_PREDICT_HC, OPT_SET,
-		  OPT_SETEPOCH, OPT_SYSTZ },
+		  OPT_GET, OPT_GETEPOCH, OPT_PREDICT_HC,
+		  OPT_SET, OPT_SETEPOCH, OPT_SYSTZ },
 		{ 'u', OPT_LOCALTIME},
 		{ OPT_ADJFILE, OPT_NOADJFILE },
 		{ 0 }
@@ -1749,7 +1723,7 @@ int main(int argc, char **argv)
 
 	/* Set option defaults */
 	show = set = systohc = hctosys = systz = adjust = noadjfile = predict =
-	    compare = FALSE;
+	    compare = get = FALSE;
 	getepoch = setepoch = utc = local_opt = directisa = testing = debug = FALSE;
 #ifdef __alpha__
 	ARCconsole = Jensen = SRM = funky_toy = badyear = FALSE;
@@ -1839,6 +1813,9 @@ int main(int argc, char **argv)
 		case OPT_PREDICT_HC:
 			predict = TRUE;		/* --predict-hc */
 			break;
+		case OPT_GET:
+			get = TRUE;		/* --get */
+			break;
 #ifdef __linux__
 		case 'f':
 			rtc_dev_name = optarg;	/* --rtc */
@@ -1896,7 +1873,7 @@ int main(int argc, char **argv)
 	}
 
 	if (!(show | set | systohc | hctosys | systz | adjust | getepoch
-	      | setepoch | predict | compare))
+	      | setepoch | predict | compare | get))
 		show = 1;	/* default to show */
 
 	if (getuid() == 0)
@@ -1953,7 +1930,7 @@ int main(int argc, char **argv)
 	} else
 		rc = manipulate_clock(show, adjust, noadjfile, set, set_time,
 			      hctosys, systohc, systz, startup_time, utc,
-			      local_opt, testing, predict);
+			      local_opt, testing, predict, get);
 
 	hwclock_exit(rc);
 	return rc;		/* Not reached */

Re: [PATCH 1/7] hwclock: hctosys drift compensation II

[Sami Kerola]

On Sat, 27 Sep 2014, JWP wrote:

> Allowing hctosys to drift compensate facilitates:
>
> More precise setting of the System Clock early in
> the boot process when --adjust cannot be used
> because the file system is not writeable.
>
> Applies sub second drift corrections immediately,
> where as --adjust cannot.
>
> Reduces boot time by not calling hwclock multiple
> times, e.g., --hctosys early before fsck when the
> file system is read-only, then --adjust later when
> the file system is read-write and --hctosys again
> for drift correction.
>
> Use of --adjust elsewhere may no longer be
> necessary.
>
> Part II
>
> After the original submission of this patch I
> realized that now all operations except --systz
> require drift corrected Hardware Clock time.
> Therefore, it should be done only once early in
> the process. Upon implementation of that premise
> many improvements were facilitated:
>
> * Adds drift correction to --hctosys.
> * Adds setting system time with sub-second precision.
> * Adds --get, a drift corrected 'show' operation.
> * Improves drift factor calculation precision while
>   greatly simplifying its algorithm.
> * Fixes --show bug, printing integer sub-seconds, and
>   now uses a more intuitive positive value.
> * Fixes --predict bug, drift correction must be
>   negated to predict future RTC time.
> * Reduces the number of function arguments and
>   lines of code.
>
> Signed-off-by: J William Piggott <elseifthen@gmx.com>
> ---
> sys-utils/hwclock.c | 205 +++++++++++++++++++++++-----------------------------
> 1 file changed, 91 insertions(+), 114 deletions(-)
>
> diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
> index 474e04f..0dd9ad6 100644
> --- a/sys-utils/hwclock.c
> +++ b/sys-utils/hwclock.c
> @@ -181,6 +181,18 @@ static void read_date_from_file(struct tm *tm)
> }
>
> /*
> + * Type time_t to type timeval conversion.
> + */
> +static struct timeval t2tv(time_t timet)

More verbose with function name would be easier to understand.

time_t_to_timeval(time_t timet)

> +{
> +  struct timeval rettimeval;
> +
> +  rettimeval.tv_sec = timet;
> +  rettimeval.tv_usec = 0;
> +  return(rettimeval);
> +}

The util-linux is using linux coding style (at least in most of the 
tools), so:

Tabs are 8 characters, and thus indentations are also 8 characters.

> +/*
>  * The difference in seconds between two times in "timeval" format.
>  */
> double time_diff(struct timeval subtrahend, struct timeval subtractor)
> @@ -678,8 +690,7 @@ set_hardware_clock_exact(const time_t sethwtime,
>  * Include in the output the adjustment "sync_duration".
>  */
> static void
> -display_time(const bool hclock_valid, const time_t systime,
> -	     const double sync_duration)
> +display_time(const bool hclock_valid, struct timeval hwctime)
> {
> 	if (!hclock_valid)
> 		warnx(_
> @@ -691,9 +702,9 @@ display_time(const bool hclock_valid, const time_t systime,
> 		char *format = "%c";
> 		char ctime_now[200];
>
> -		lt = localtime(&systime);
> +		lt = localtime(&hwctime.tv_sec);
> 		strftime(ctime_now, sizeof(ctime_now), format, lt);
> -		printf(_("%s  %.6f seconds\n"), ctime_now, -(sync_duration));
> +		printf(_("%s  .%06d seconds\n"), ctime_now, (int)hwctime.tv_usec);

Why not to avoid cast, and print by using the type the tv_usec is?

printf(_("%s  .%06ld seconds\n"), ctime_now, hwctime.tv_usec);

> 	}
> }
>
> @@ -807,7 +818,7 @@ static int interpret_date_string(const char *date_opt, time_t * const time_p)
>  * have.
>  */
> static int
> -set_system_clock(const bool hclock_valid, const time_t newtime,
> +set_system_clock(const bool hclock_valid, const struct timeval newtime,
> 		 const bool testing)
> {
> 	int retcode;
> @@ -818,15 +829,11 @@ set_system_clock(const bool hclock_valid, const time_t newtime,
> 		       "we cannot set the System Time from it."));
> 		retcode = 1;
> 	} else {
> -		struct timeval tv;
> 		struct tm *broken;
> 		int minuteswest;
> 		int rc;
>
> -		tv.tv_sec = newtime;
> -		tv.tv_usec = 0;
> -
> -		broken = localtime(&newtime);
> +		broken = localtime(&newtime.tv_sec);
> #ifdef HAVE_TM_GMTOFF
> 		minuteswest = -broken->tm_gmtoff / 60;	/* GNU extension */
> #else
> @@ -838,7 +845,7 @@ set_system_clock(const bool hclock_valid, const time_t newtime,
> 		if (debug) {
> 			printf(_("Calling settimeofday:\n"));
> 			printf(_("\ttv.tv_sec = %ld, tv.tv_usec = %ld\n"),
> -			       (long)tv.tv_sec, (long)tv.tv_usec);
> +			       (long)newtime.tv_sec, (long)newtime.tv_usec);

casting to long seems unnecessary

> 			printf(_("\ttz.tz_minuteswest = %d\n"), minuteswest);
> 		}
> 		if (testing) {
> @@ -848,7 +855,7 @@ set_system_clock(const bool hclock_valid, const time_t newtime,
> 		} else {
> 			const struct timezone tz = { minuteswest, 0 };
>
> -			rc = settimeofday(&tv, &tz);
> +			rc = settimeofday(&newtime, &tz);
> 			if (rc) {
> 				if (errno == EPERM) {
> 					warnx(_
> @@ -974,9 +981,9 @@ static int set_system_clock_timezone(const bool universal, const bool testing)
>  */
> static void
> adjust_drift_factor(struct adjtime *adjtime_p,
> -		    const time_t nowtime,
> +		    const struct timeval nowtime,
> 		    const bool hclock_valid,
> -		    const time_t hclocktime, const double sync_delay)
> +		    const struct timeval hclocktime)
> {
> 	if (!hclock_valid) {
> 		if (debug)
> @@ -989,7 +996,7 @@ adjust_drift_factor(struct adjtime *adjtime_p,
> 				 "calibration time is zero,\n"
> 				 "so history is bad and calibration startover "
> 				 "is necessary.\n"));
> -	} else if ((hclocktime - adjtime_p->last_calib_time) < 24 * 60 * 60) {
> +	} else if ((hclocktime.tv_sec - adjtime_p->last_calib_time) < 24 * 60 * 60) {
> 		if (debug)
> 			printf(_("Not adjusting drift factor because it has "
> 				 "been less than a day since the last "
> @@ -1009,39 +1016,16 @@ adjust_drift_factor(struct adjtime *adjtime_p,
> 		 * but 195 days + 1 second equals 195 days in floats.)
> 		 */
> 		const double sec_per_day = 24.0 * 60.0 * 60.0;
> -		double atime_per_htime;
> -		double adj_days, cal_days;
> -		double exp_drift, unc_drift;
> 		double factor_adjust;
> 		double drift_factor;
> +	        struct timeval lastCalib;

s/lastCalib/last_calib/

CamelCases are unexpected. When fixing that change also 8 spaces in front 
of the variable to one tab.

>
> -		/* Adjusted time units per hardware time unit */
> -		atime_per_htime = 1.0 + adjtime_p->drift_factor / sec_per_day;
> +		lastCalib = t2tv(adjtime_p->last_calib_time);
>
> -		/* Days since last adjustment (in hardware clock time) */
> -		adj_days = (double)(hclocktime - adjtime_p->last_adj_time)
> -		    / sec_per_day;
> +		factor_adjust = time_diff(nowtime, hclocktime) /
> +				(time_diff(nowtime, lastCalib) / sec_per_day);
>
> -		/* Expected drift (sec) since last adjustment */
> -		exp_drift = adj_days * adjtime_p->drift_factor
> -		    + adjtime_p->not_adjusted;
> -
> -		/* Uncorrected drift (sec) since last calibration */
> -		unc_drift = (double)(nowtime - hclocktime)
> -		    + sync_delay - exp_drift;
> -
> -		/* Days since last calibration (in hardware clock time) */
> -		cal_days = ((double)(adjtime_p->last_adj_time
> -				     - adjtime_p->last_calib_time)
> -			    + adjtime_p->not_adjusted)
> -		    / (sec_per_day * atime_per_htime) + adj_days;
> -
> -		/* Amount to add to previous drift factor */
> -		factor_adjust = unc_drift / cal_days;
> -
> -		/* New drift factor */
> 		drift_factor = adjtime_p->drift_factor + factor_adjust;
> -
> 		if (abs(drift_factor) > MAX_DRIFT) {
> 			if (debug)
> 				printf(_("Clock drift factor was calculated as "
> @@ -1052,19 +1036,19 @@ adjust_drift_factor(struct adjtime *adjtime_p,
> 		} else {
> 			if (debug)
> 				printf(_("Clock drifted %.1f seconds in the past "
> -					 "%d seconds in spite of a drift factor of "
> +					 "%.1f seconds\nin spite of a drift factor of "
> 					 "%f seconds/day.\n"
> 					 "Adjusting drift factor by %f seconds/day\n"),
> -				       unc_drift,
> -				       (int)(nowtime - adjtime_p->last_calib_time),
> +				       time_diff(nowtime, hclocktime),
> +				       time_diff(nowtime, lastCalib),
> 				       adjtime_p->drift_factor, factor_adjust);
> 		}
>
> 		adjtime_p->drift_factor = drift_factor;
> 	}
> -	adjtime_p->last_calib_time = nowtime;
> +	adjtime_p->last_calib_time = nowtime.tv_sec;
>
> -	adjtime_p->last_adj_time = nowtime;
> +	adjtime_p->last_adj_time = nowtime.tv_sec;
>
> 	adjtime_p->not_adjusted = 0;
>
> @@ -1087,26 +1071,23 @@ static void
> calculate_adjustment(const double factor,
> 		     const time_t last_time,
> 		     const double not_adjusted,
> -		     const time_t systime, int *adjustment_p, double *retro_p)
> +		     const time_t systime, struct timeval *tdrift_p)
> {
> 	double exact_adjustment;
>
> 	exact_adjustment =
> 	    ((double)(systime - last_time)) * factor / (24 * 60 * 60)
> 	    + not_adjusted;
> -	*adjustment_p = FLOOR(exact_adjustment);
> -
> -	*retro_p = exact_adjustment - (double)*adjustment_p;
> +	tdrift_p->tv_sec = FLOOR(exact_adjustment);
> +	tdrift_p->tv_usec = (exact_adjustment -
> +				 (double)tdrift_p->tv_sec) * 1E6;
> 	if (debug) {
> 		printf(P_("Time since last adjustment is %d second\n",
> 			"Time since last adjustment is %d seconds\n",
> 		       (int)(systime - last_time)),
> 		       (int)(systime - last_time));
> -		printf(P_("Need to insert %d second and refer time back "
> -			 "%.6f seconds ago\n",
> -			 "Need to insert %d seconds and refer time back "
> -			 "%.6f seconds ago\n", *adjustment_p),
> -			 *adjustment_p, *retro_p);
> +		printf(_("Calculated Hardware Clock drift is %ld.%06d seconds\n"),
> +		       (long)tdrift_p->tv_sec, (int)tdrift_p->tv_usec);
> 	}
> }
>
> @@ -1200,7 +1181,7 @@ static void save_adjtime(const struct adjtime adjtime, const bool testing)
>  */
> static void
> do_adjustment(struct adjtime *adjtime_p,
> -	      const bool hclock_valid, const time_t hclocktime,
> +	      const bool hclock_valid, const struct timeval hclocktime,
> 	      const struct timeval read_time,
> 	      const bool universal, const bool testing)
> {
> @@ -1220,27 +1201,13 @@ do_adjustment(struct adjtime *adjtime_p,
> 			printf(_("Not setting clock because drift factor %f is far too high.\n"),
> 				adjtime_p->drift_factor);
> 	} else {
> -		int adjustment;
> -		/* Number of seconds we must insert in the Hardware Clock */
> -		double retro;
> -		/*
> -		 * Fraction of second we have to remove from clock after
> -		 * inserting <adjustment> whole seconds.
> -		 */
> -		calculate_adjustment(adjtime_p->drift_factor,
> -				     adjtime_p->last_adj_time,
> -				     adjtime_p->not_adjusted,
> -				     hclocktime, &adjustment, &retro);
> -		if (adjustment > 0 || adjustment < -1) {
> -			set_hardware_clock_exact(hclocktime + adjustment,
> -						 time_inc(read_time, -retro),
> -						 universal, testing);
> -			adjtime_p->last_adj_time = hclocktime + adjustment;
> -			adjtime_p->not_adjusted = 0;
> -			adjtime_p->dirty = TRUE;
> -		} else if (debug)
> -			printf(_("Needed adjustment is less than one second, "
> -				 "so not setting clock.\n"));
> +		set_hardware_clock_exact(hclocktime.tv_sec,
> +					 time_inc(read_time,
> +						  -(hclocktime.tv_usec / 1E6)),
> +					 universal, testing);
> +		adjtime_p->last_adj_time = hclocktime.tv_sec;
> +		adjtime_p->not_adjusted = 0;
> +		adjtime_p->dirty = TRUE;
> 	}
> }
>
> @@ -1283,7 +1250,7 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
> 		 const bool hctosys, const bool systohc, const bool systz,
> 		 const struct timeval startup_time,
> 		 const bool utc, const bool local_opt,
> -		 const bool testing, const bool predict)
> +		 const bool testing, const bool predict, const bool get)
> {
> 	/* Contents of the adjtime file, or what they should be. */
> 	struct adjtime adjtime;
> @@ -1302,7 +1269,8 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
> 	 * synchronized to its next clock tick when we
> 	 * started up. Defined only if hclock_valid is true.
> 	 */
> -	time_t hclocktime = 0;
> +	struct timeval hclocktime = { 0, 0 };
> +	struct timeval tdrift;
> 	/* local return code */
> 	int rc = 0;
>
> @@ -1312,13 +1280,12 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
> 			return EX_NOPERM;
> 	}
>
> -	if (!noadjfile
> -	    && (adjust || set || systohc || (!utc && !local_opt) || predict)) {
> +	if (!noadjfile && !(systz && (utc || local_opt))) {
> 		rc = read_adjtime(&adjtime);
> 		if (rc)
> 			return rc;
> 	} else {
> -		/* A little trick to avoid reading the file if we don't have to */
> +		/* A little trick to avoid writing the file if we don't have to */
> 		adjtime.dirty = FALSE;
> 	}
>
> @@ -1330,7 +1297,7 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
> 		adjtime.dirty = TRUE;
> 	}
>
> -	if (show || adjust || hctosys || (!noadjfile && !systz && !predict)) {
> +	if (show || get || adjust || hctosys || (!noadjfile && !systz && !predict)) {
> 		/* data from HW-clock are required */
> 		rc = synchronize_to_clock_tick();
>
> @@ -1353,26 +1320,40 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
> 		 */
> 		if (!rc) {
> 			rc = read_hardware_clock(universal,
> -						 &hclock_valid, &hclocktime);
> +						 &hclock_valid, &hclocktime.tv_sec);
> 			if (rc && !set && !systohc)
> 				return EX_IOERR;
> 		}
> 	}
> -
> -	if (show) {
> -		display_time(hclock_valid, hclocktime,
> -			     time_diff(read_time, startup_time));
> +	hclocktime = predict ? t2tv(set_time) : hclocktime;
> +	calculate_adjustment(adjtime.drift_factor,
> +			     adjtime.last_adj_time,
> +			     adjtime.not_adjusted,
> +			     hclocktime.tv_sec, &tdrift);
> +	if (!show && !predict)
> +		hclocktime = time_inc(tdrift, hclocktime.tv_sec);
> +	if (predict)
> +		hclocktime = time_inc(hclocktime, (double)
> +				      -(tdrift.tv_sec + tdrift.tv_sec / 1E6));
> +	if (show || get) {
> +		display_time(hclock_valid,
> +			     time_inc(hclocktime, -time_diff
> +				      (read_time, startup_time)));
> 	} else if (set) {
> 		set_hardware_clock_exact(set_time, startup_time,
> 					 universal, testing);
> 		if (!noadjfile)
> -			adjust_drift_factor(&adjtime, set_time,
> -					    hclock_valid,
> -					    hclocktime,
> -					    time_diff(read_time, startup_time));
> +			adjust_drift_factor(&adjtime,
> +					    time_inc(t2tv(set_time), time_diff
> +						     (read_time, startup_time)),
> +					    hclock_valid, hclocktime);
> 	} else if (adjust) {
> -		do_adjustment(&adjtime, hclock_valid,
> -			      hclocktime, read_time, universal, testing);
> +		if (tdrift.tv_sec > 0 || tdrift.tv_sec < -1)
> +			do_adjustment(&adjtime, hclock_valid,
> +				      hclocktime, read_time, universal, testing);
> +		else
> +			printf(_("Needed adjustment is less than one second, "
> +				 "so not setting clock.\n"));
> 	} else if (systohc) {
> 		struct timeval nowtime, reftime;
> 		/*
> @@ -1388,10 +1369,8 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
> 					 reftime.tv_sec,
> 					 reftime, universal, testing);
> 		if (!noadjfile)
> -			adjust_drift_factor(&adjtime, (time_t)
> -					    reftime.tv_sec,
> -					    hclock_valid, hclocktime, (double)
> -					    read_time.tv_usec / 1E6);
> +			adjust_drift_factor(&adjtime, nowtime,
> +					    hclock_valid, hclocktime);
> 	} else if (hctosys) {
> 		rc = set_system_clock(hclock_valid, hclocktime, testing);
> 		if (rc) {
> @@ -1405,19 +1384,12 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
> 			return rc;
> 		}
> 	} else if (predict) {
> -		int adjustment;
> -		double retro;
> -
> -		calculate_adjustment(adjtime.drift_factor,
> -				     adjtime.last_adj_time,
> -				     adjtime.not_adjusted,
> -				     set_time, &adjustment, &retro);
> 		if (debug) {
> 			printf(_
> 			       ("At %ld seconds after 1969, RTC is predicted to read %ld seconds after 1969.\n"),
> -			       set_time, set_time + adjustment);
> +			       set_time, (long)hclocktime.tv_sec);
> 		}
> -		display_time(TRUE, set_time + adjustment, -retro);
> +		display_time(TRUE, hclocktime);
> 	}
> 	if (!noadjfile)
> 		save_adjtime(adjtime, testing);
> @@ -1646,7 +1618,7 @@ int main(int argc, char **argv)
> 	/* Variables set by various options; show may also be set later */
> 	/* The options debug, badyear and epoch_option are global */
> 	bool show, set, systohc, hctosys, systz, adjust, getepoch, setepoch,
> -	    predict, compare;
> +	    predict, compare, get;
> 	bool utc, testing, local_opt, noadjfile, directisa;
> 	char *date_opt;
> #ifdef __alpha__
> @@ -1659,6 +1631,7 @@ int main(int argc, char **argv)
> 		OPT_DATE,
> 		OPT_DIRECTISA,
> 		OPT_EPOCH,
> +		OPT_GET,
> 		OPT_GETEPOCH,
> 		OPT_LOCALTIME,
> 		OPT_NOADJFILE,
> @@ -1706,13 +1679,14 @@ int main(int argc, char **argv)
> 		{"adjfile",	1, 0, OPT_ADJFILE},
> 		{"systz",	0, 0, OPT_SYSTZ},
> 		{"predict-hc",	0, 0, OPT_PREDICT_HC},
> +		{"get",		0, 0, OPT_GET},
> 		{NULL,		0, NULL, 0}
> 	};
>
> 	static const ul_excl_t excl[] = {	/* rows and cols in in ASCII order */
> 		{ 'a','r','s','w',
> -		  OPT_GETEPOCH,	OPT_PREDICT_HC, OPT_SET,
> -		  OPT_SETEPOCH, OPT_SYSTZ },
> +		  OPT_GET, OPT_GETEPOCH, OPT_PREDICT_HC,
> +		  OPT_SET, OPT_SETEPOCH, OPT_SYSTZ },
> 		{ 'u', OPT_LOCALTIME},
> 		{ OPT_ADJFILE, OPT_NOADJFILE },
> 		{ 0 }
> @@ -1749,7 +1723,7 @@ int main(int argc, char **argv)
>
> 	/* Set option defaults */
> 	show = set = systohc = hctosys = systz = adjust = noadjfile = predict =
> -	    compare = FALSE;
> +	    compare = get = FALSE;
> 	getepoch = setepoch = utc = local_opt = directisa = testing = debug = FALSE;
> #ifdef __alpha__
> 	ARCconsole = Jensen = SRM = funky_toy = badyear = FALSE;
> @@ -1839,6 +1813,9 @@ int main(int argc, char **argv)
> 		case OPT_PREDICT_HC:
> 			predict = TRUE;		/* --predict-hc */
> 			break;
> +		case OPT_GET:
> +			get = TRUE;		/* --get */
> +			break;
> #ifdef __linux__
> 		case 'f':
> 			rtc_dev_name = optarg;	/* --rtc */
> @@ -1896,7 +1873,7 @@ int main(int argc, char **argv)
> 	}
>
> 	if (!(show | set | systohc | hctosys | systz | adjust | getepoch
> -	      | setepoch | predict | compare))
> +	      | setepoch | predict | compare | get))
> 		show = 1;	/* default to show */
>
> 	if (getuid() == 0)
> @@ -1953,7 +1930,7 @@ int main(int argc, char **argv)
> 	} else
> 		rc = manipulate_clock(show, adjust, noadjfile, set, set_time,
> 			      hctosys, systohc, systz, startup_time, utc,
> -			      local_opt, testing, predict);
> +			      local_opt, testing, predict, get);
>
> 	hwclock_exit(rc);
> 	return rc;		/* Not reached */

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

Re: [PATCH 1/7] hwclock: hctosys drift compensation II

[JWP]

Hello Sami,

Thank you for taking time to review my code.

As I am new here I'd like to take a moment to say that
all of my replies to the project are intended to be 
conversational, and not argumentative. I hope they will 
be received in that tone and spirit. 

On 09/28/2014 01:55 PM, Sami Kerola wrote:
>On Sat, 27 Sep 2014, JWP wrote:
>
>> Allowing hctosys to drift compensate facilitates:
>>
>> More precise setting of the System Clock early in
>> the boot process when --adjust cannot be used
>> because the file system is not writeable.
>>
>> Applies sub second drift corrections immediately,
>> where as --adjust cannot.
>>
>> Reduces boot time by not calling hwclock multiple
>> times, e.g., --hctosys early before fsck when the
>> file system is read-only, then --adjust later when
>> the file system is read-write and --hctosys again
>> for drift correction.
>>
>> Use of --adjust elsewhere may no longer be
>> necessary.
>>
>> Part II
>>
>> After the original submission of this patch I
>> realized that now all operations except --systz
>> require drift corrected Hardware Clock time.
>> Therefore, it should be done only once early in
>> the process. Upon implementation of that premise
>> many improvements were facilitated:
>>
>> * Adds drift correction to --hctosys.
>> * Adds setting system time with sub-second precision.
>> * Adds --get, a drift corrected 'show' operation.
>> * Improves drift factor calculation precision while
>>   greatly simplifying its algorithm.
>> * Fixes --show bug, printing integer sub-seconds, and
>>   now uses a more intuitive positive value.
>> * Fixes --predict bug, drift correction must be
>>   negated to predict future RTC time.
>> * Reduces the number of function arguments and
>>   lines of code.
>>
>> Signed-off-by: J William Piggott <elseifthen@gmx.com>
>> ---
>> sys-utils/hwclock.c | 205 +++++++++++++++++++++++-----------------------------
>> 1 file changed, 91 insertions(+), 114 deletions(-)
>>
>> diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
>> index 474e04f..0dd9ad6 100644
>> --- a/sys-utils/hwclock.c
>> +++ b/sys-utils/hwclock.c
>> @@ -181,6 +181,18 @@ static void read_date_from_file(struct tm *tm)
>> }
>>
>> /*
>> + * Type time_t to type timeval conversion.
>> + */
>> +static struct timeval t2tv(time_t timet)
>
>More verbose with function name would be easier to understand.
>
>time_t_to_timeval(time_t timet)
>

I would normally agree, but I think this case is an exception for
the same reason that all typecasting labels should be short. Imagine
how ugly the code would look if (int) were (typecast_to_integer). 

Because time_t and timeval are used extensively throughout this code 
I think it should be clear to the reader what t2tv() means.

Having said that, if the project and Mr. Zak believe a longer name is 
preferred then I will happily go with whatever name is decided upon. 

>> +{
>> +  struct timeval rettimeval;
>> +
>> +  rettimeval.tv_sec = timet;
>> +  rettimeval.tv_usec = 0;
>> +  return(rettimeval);
>> +}
>
>The util-linux is using linux coding style (at least in most of the 
>tools), so:
>
>Tabs are 8 characters, and thus indentations are also 8 characters.
>

Well, that's embarrassing.  I thought I had git configured to highlight
those mistakes for me. My apologizes for such a stupid error. 

>> +/*
>>  * The difference in seconds between two times in "timeval" format.
>>  */
>> double time_diff(struct timeval subtrahend, struct timeval subtractor)
>> @@ -678,8 +690,7 @@ set_hardware_clock_exact(const time_t sethwtime,
>>  * Include in the output the adjustment "sync_duration".
>>  */
>> static void
>> -display_time(const bool hclock_valid, const time_t systime,
>> -	     const double sync_duration)
>> +display_time(const bool hclock_valid, struct timeval hwctime)
>> {
>> 	if (!hclock_valid)
>> 		warnx(_
>> @@ -691,9 +702,9 @@ display_time(const bool hclock_valid, const time_t systime,
>> 		char *format = "%c";
>> 		char ctime_now[200];
>>
>> -		lt = localtime(&systime);
>> +		lt = localtime(&hwctime.tv_sec);
>> 		strftime(ctime_now, sizeof(ctime_now), format, lt);
>> -		printf(_("%s  %.6f seconds\n"), ctime_now, -(sync_duration));
>> +		printf(_("%s  .%06d seconds\n"), ctime_now, (int)hwctime.tv_usec);
>
>Why not to avoid cast, and print by using the type the tv_usec is?
>
>printf(_("%s  .%06ld seconds\n"), ctime_now, hwctime.tv_usec);


I was following the existing practice.  All of the print statements
in this code use typecasting.  I would suggest, rather than breaking
code continuity by modifying the few instances in this patch, that a
separate patch changing all occurrences would be preferred? I would 
be happy to add that to my todo list if the project wants it?


>> 	}
>> }
>>
>> @@ -807,7 +818,7 @@ static int interpret_date_string(const char *date_opt, time_t * const time_p)
>>  * have.
>>  */
>> static int
>> -set_system_clock(const bool hclock_valid, const time_t newtime,
>> +set_system_clock(const bool hclock_valid, const struct timeval newtime,
>> 		 const bool testing)
>> {
>> 	int retcode;
>> @@ -818,15 +829,11 @@ set_system_clock(const bool hclock_valid, const time_t newtime,
>> 		       "we cannot set the System Time from it."));
>> 		retcode = 1;
>> 	} else {
>> -		struct timeval tv;
>> 		struct tm *broken;
>> 		int minuteswest;
>> 		int rc;
>>
>> -		tv.tv_sec = newtime;
>> -		tv.tv_usec = 0;
>> -
>> -		broken = localtime(&newtime);
>> +		broken = localtime(&newtime.tv_sec);
>> #ifdef HAVE_TM_GMTOFF
>> 		minuteswest = -broken->tm_gmtoff / 60;	/* GNU extension */
>> #else
>> @@ -838,7 +845,7 @@ set_system_clock(const bool hclock_valid, const time_t newtime,
>> 		if (debug) {
>> 			printf(_("Calling settimeofday:\n"));
>> 			printf(_("\ttv.tv_sec = %ld, tv.tv_usec = %ld\n"),
>> -			       (long)tv.tv_sec, (long)tv.tv_usec);
>> +			       (long)newtime.tv_sec, (long)newtime.tv_usec);
>
>casting to long seems unnecessary

I was following existing practices.
Same reasoning as my previous response.


>
>> 			printf(_("\ttz.tz_minuteswest = %d\n"), minuteswest);
>> 		}
>> 		if (testing) {
>> @@ -848,7 +855,7 @@ set_system_clock(const bool hclock_valid, const time_t newtime,
>> 		} else {
>> 			const struct timezone tz = { minuteswest, 0 };
>>
>> -			rc = settimeofday(&tv, &tz);
>> +			rc = settimeofday(&newtime, &tz);
>> 			if (rc) {
>> 				if (errno == EPERM) {
>> 					warnx(_
>> @@ -974,9 +981,9 @@ static int set_system_clock_timezone(const bool universal, const bool testing)
>>  */
>> static void
>> adjust_drift_factor(struct adjtime *adjtime_p,
>> -		    const time_t nowtime,
>> +		    const struct timeval nowtime,
>> 		    const bool hclock_valid,
>> -		    const time_t hclocktime, const double sync_delay)
>> +		    const struct timeval hclocktime)
>> {
>> 	if (!hclock_valid) {
>> 		if (debug)
>> @@ -989,7 +996,7 @@ adjust_drift_factor(struct adjtime *adjtime_p,
>> 				 "calibration time is zero,\n"
>> 				 "so history is bad and calibration startover "
>> 				 "is necessary.\n"));
>> -	} else if ((hclocktime - adjtime_p->last_calib_time) < 24 * 60 * 60) {
>> +	} else if ((hclocktime.tv_sec - adjtime_p->last_calib_time) < 24 * 60 * 60) {
>> 		if (debug)
>> 			printf(_("Not adjusting drift factor because it has "
>> 				 "been less than a day since the last "
>> @@ -1009,39 +1016,16 @@ adjust_drift_factor(struct adjtime *adjtime_p,
>> 		 * but 195 days + 1 second equals 195 days in floats.)
>> 		 */
>> 		const double sec_per_day = 24.0 * 60.0 * 60.0;
>> -		double atime_per_htime;
>> -		double adj_days, cal_days;
>> -		double exp_drift, unc_drift;
>> 		double factor_adjust;
>> 		double drift_factor;
>> +	        struct timeval lastCalib;
>
>s/lastCalib/last_calib/
>
>CamelCases are unexpected. When fixing that change also 8 spaces in front 
>of the variable to one tab.
>

Very true, more dumb mistakes on my part.

Mr. Zak, could you please tell me how to proceed on these changes?
Should they all be implemented as Mr. Kerola wrote them?
Do you want to tweak the code, or do you want me to resubmit a 
single patch, or the entire series?

Thank you.

>>
>> -		/* Adjusted time units per hardware time unit */
>> -		atime_per_htime = 1.0 + adjtime_p->drift_factor / sec_per_day;
>> +		lastCalib = t2tv(adjtime_p->last_calib_time);
>>
>> -		/* Days since last adjustment (in hardware clock time) */
>> -		adj_days = (double)(hclocktime - adjtime_p->last_adj_time)
>> -		    / sec_per_day;
>> +		factor_adjust = time_diff(nowtime, hclocktime) /
>> +				(time_diff(nowtime, lastCalib) / sec_per_day);
>>
>> -		/* Expected drift (sec) since last adjustment */
>> -		exp_drift = adj_days * adjtime_p->drift_factor
>> -		    + adjtime_p->not_adjusted;
>> -
>> -		/* Uncorrected drift (sec) since last calibration */
>> -		unc_drift = (double)(nowtime - hclocktime)
>> -		    + sync_delay - exp_drift;
>> -
>> -		/* Days since last calibration (in hardware clock time) */
>> -		cal_days = ((double)(adjtime_p->last_adj_time
>> -				     - adjtime_p->last_calib_time)
>> -			    + adjtime_p->not_adjusted)
>> -		    / (sec_per_day * atime_per_htime) + adj_days;
>> -
>> -		/* Amount to add to previous drift factor */
>> -		factor_adjust = unc_drift / cal_days;
>> -
>> -		/* New drift factor */
>> 		drift_factor = adjtime_p->drift_factor + factor_adjust;
>> -
>> 		if (abs(drift_factor) > MAX_DRIFT) {
>> 			if (debug)
>> 				printf(_("Clock drift factor was calculated as "
>> @@ -1052,19 +1036,19 @@ adjust_drift_factor(struct adjtime *adjtime_p,
>> 		} else {
>> 			if (debug)
>> 				printf(_("Clock drifted %.1f seconds in the past "
>> -					 "%d seconds in spite of a drift factor of "
>> +					 "%.1f seconds\nin spite of a drift factor of "
>> 					 "%f seconds/day.\n"
>> 					 "Adjusting drift factor by %f seconds/day\n"),
>> -				       unc_drift,
>> -				       (int)(nowtime - adjtime_p->last_calib_time),
>> +				       time_diff(nowtime, hclocktime),
>> +				       time_diff(nowtime, lastCalib),
>> 				       adjtime_p->drift_factor, factor_adjust);
>> 		}
>>
>> 		adjtime_p->drift_factor = drift_factor;
>> 	}
>> -	adjtime_p->last_calib_time = nowtime;
>> +	adjtime_p->last_calib_time = nowtime.tv_sec;
>>
>> -	adjtime_p->last_adj_time = nowtime;
>> +	adjtime_p->last_adj_time = nowtime.tv_sec;
>>
>> 	adjtime_p->not_adjusted = 0;
>>
>> @@ -1087,26 +1071,23 @@ static void
>> calculate_adjustment(const double factor,
>> 		     const time_t last_time,
>> 		     const double not_adjusted,
>> -		     const time_t systime, int *adjustment_p, double *retro_p)
>> +		     const time_t systime, struct timeval *tdrift_p)
>> {
>> 	double exact_adjustment;
>>
>> 	exact_adjustment =
>> 	    ((double)(systime - last_time)) * factor / (24 * 60 * 60)
>> 	    + not_adjusted;
>> -	*adjustment_p = FLOOR(exact_adjustment);
>> -
>> -	*retro_p = exact_adjustment - (double)*adjustment_p;
>> +	tdrift_p->tv_sec = FLOOR(exact_adjustment);
>> +	tdrift_p->tv_usec = (exact_adjustment -
>> +				 (double)tdrift_p->tv_sec) * 1E6;
>> 	if (debug) {
>> 		printf(P_("Time since last adjustment is %d second\n",
>> 			"Time since last adjustment is %d seconds\n",
>> 		       (int)(systime - last_time)),
>> 		       (int)(systime - last_time));
>> -		printf(P_("Need to insert %d second and refer time back "
>> -			 "%.6f seconds ago\n",
>> -			 "Need to insert %d seconds and refer time back "
>> -			 "%.6f seconds ago\n", *adjustment_p),
>> -			 *adjustment_p, *retro_p);
>> +		printf(_("Calculated Hardware Clock drift is %ld.%06d seconds\n"),
>> +		       (long)tdrift_p->tv_sec, (int)tdrift_p->tv_usec);
>> 	}
>> }
>>
>> @@ -1200,7 +1181,7 @@ static void save_adjtime(const struct adjtime adjtime, const bool testing)
>>  */
>> static void
>> do_adjustment(struct adjtime *adjtime_p,
>> -	      const bool hclock_valid, const time_t hclocktime,
>> +	      const bool hclock_valid, const struct timeval hclocktime,
>> 	      const struct timeval read_time,
>> 	      const bool universal, const bool testing)
>> {
>> @@ -1220,27 +1201,13 @@ do_adjustment(struct adjtime *adjtime_p,
>> 			printf(_("Not setting clock because drift factor %f is far too high.\n"),
>> 				adjtime_p->drift_factor);
>> 	} else {
>> -		int adjustment;
>> -		/* Number of seconds we must insert in the Hardware Clock */
>> -		double retro;
>> -		/*
>> -		 * Fraction of second we have to remove from clock after
>> -		 * inserting <adjustment> whole seconds.
>> -		 */
>> -		calculate_adjustment(adjtime_p->drift_factor,
>> -				     adjtime_p->last_adj_time,
>> -				     adjtime_p->not_adjusted,
>> -				     hclocktime, &adjustment, &retro);
>> -		if (adjustment > 0 || adjustment < -1) {
>> -			set_hardware_clock_exact(hclocktime + adjustment,
>> -						 time_inc(read_time, -retro),
>> -						 universal, testing);
>> -			adjtime_p->last_adj_time = hclocktime + adjustment;
>> -			adjtime_p->not_adjusted = 0;
>> -			adjtime_p->dirty = TRUE;
>> -		} else if (debug)
>> -			printf(_("Needed adjustment is less than one second, "
>> -				 "so not setting clock.\n"));
>> +		set_hardware_clock_exact(hclocktime.tv_sec,
>> +					 time_inc(read_time,
>> +						  -(hclocktime.tv_usec / 1E6)),
>> +					 universal, testing);
>> +		adjtime_p->last_adj_time = hclocktime.tv_sec;
>> +		adjtime_p->not_adjusted = 0;
>> +		adjtime_p->dirty = TRUE;
>> 	}
>> }
>>
>> @@ -1283,7 +1250,7 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
>> 		 const bool hctosys, const bool systohc, const bool systz,
>> 		 const struct timeval startup_time,
>> 		 const bool utc, const bool local_opt,
>> -		 const bool testing, const bool predict)
>> +		 const bool testing, const bool predict, const bool get)
>> {
>> 	/* Contents of the adjtime file, or what they should be. */
>> 	struct adjtime adjtime;
>> @@ -1302,7 +1269,8 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
>> 	 * synchronized to its next clock tick when we
>> 	 * started up. Defined only if hclock_valid is true.
>> 	 */
>> -	time_t hclocktime = 0;
>> +	struct timeval hclocktime = { 0, 0 };
>> +	struct timeval tdrift;
>> 	/* local return code */
>> 	int rc = 0;
>>
>> @@ -1312,13 +1280,12 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
>> 			return EX_NOPERM;
>> 	}
>>
>> -	if (!noadjfile
>> -	    && (adjust || set || systohc || (!utc && !local_opt) || predict)) {
>> +	if (!noadjfile && !(systz && (utc || local_opt))) {
>> 		rc = read_adjtime(&adjtime);
>> 		if (rc)
>> 			return rc;
>> 	} else {
>> -		/* A little trick to avoid reading the file if we don't have to */
>> +		/* A little trick to avoid writing the file if we don't have to */
>> 		adjtime.dirty = FALSE;
>> 	}
>>
>> @@ -1330,7 +1297,7 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
>> 		adjtime.dirty = TRUE;
>> 	}
>>
>> -	if (show || adjust || hctosys || (!noadjfile && !systz && !predict)) {
>> +	if (show || get || adjust || hctosys || (!noadjfile && !systz && !predict)) {
>> 		/* data from HW-clock are required */
>> 		rc = synchronize_to_clock_tick();
>>
>> @@ -1353,26 +1320,40 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
>> 		 */
>> 		if (!rc) {
>> 			rc = read_hardware_clock(universal,
>> -						 &hclock_valid, &hclocktime);
>> +						 &hclock_valid, &hclocktime.tv_sec);
>> 			if (rc && !set && !systohc)
>> 				return EX_IOERR;
>> 		}
>> 	}
>> -
>> -	if (show) {
>> -		display_time(hclock_valid, hclocktime,
>> -			     time_diff(read_time, startup_time));
>> +	hclocktime = predict ? t2tv(set_time) : hclocktime;
>> +	calculate_adjustment(adjtime.drift_factor,
>> +			     adjtime.last_adj_time,
>> +			     adjtime.not_adjusted,
>> +			     hclocktime.tv_sec, &tdrift);
>> +	if (!show && !predict)
>> +		hclocktime = time_inc(tdrift, hclocktime.tv_sec);
>> +	if (predict)
>> +		hclocktime = time_inc(hclocktime, (double)
>> +				      -(tdrift.tv_sec + tdrift.tv_sec / 1E6));
>> +	if (show || get) {
>> +		display_time(hclock_valid,
>> +			     time_inc(hclocktime, -time_diff
>> +				      (read_time, startup_time)));
>> 	} else if (set) {
>> 		set_hardware_clock_exact(set_time, startup_time,
>> 					 universal, testing);
>> 		if (!noadjfile)
>> -			adjust_drift_factor(&adjtime, set_time,
>> -					    hclock_valid,
>> -					    hclocktime,
>> -					    time_diff(read_time, startup_time));
>> +			adjust_drift_factor(&adjtime,
>> +					    time_inc(t2tv(set_time), time_diff
>> +						     (read_time, startup_time)),
>> +					    hclock_valid, hclocktime);
>> 	} else if (adjust) {
>> -		do_adjustment(&adjtime, hclock_valid,
>> -			      hclocktime, read_time, universal, testing);
>> +		if (tdrift.tv_sec > 0 || tdrift.tv_sec < -1)
>> +			do_adjustment(&adjtime, hclock_valid,
>> +				      hclocktime, read_time, universal, testing);
>> +		else
>> +			printf(_("Needed adjustment is less than one second, "
>> +				 "so not setting clock.\n"));
>> 	} else if (systohc) {
>> 		struct timeval nowtime, reftime;
>> 		/*
>> @@ -1388,10 +1369,8 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
>> 					 reftime.tv_sec,
>> 					 reftime, universal, testing);
>> 		if (!noadjfile)
>> -			adjust_drift_factor(&adjtime, (time_t)
>> -					    reftime.tv_sec,
>> -					    hclock_valid, hclocktime, (double)
>> -					    read_time.tv_usec / 1E6);
>> +			adjust_drift_factor(&adjtime, nowtime,
>> +					    hclock_valid, hclocktime);
>> 	} else if (hctosys) {
>> 		rc = set_system_clock(hclock_valid, hclocktime, testing);
>> 		if (rc) {
>> @@ -1405,19 +1384,12 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
>> 			return rc;
>> 		}
>> 	} else if (predict) {
>> -		int adjustment;
>> -		double retro;
>> -
>> -		calculate_adjustment(adjtime.drift_factor,
>> -				     adjtime.last_adj_time,
>> -				     adjtime.not_adjusted,
>> -				     set_time, &adjustment, &retro);
>> 		if (debug) {
>> 			printf(_
>> 			       ("At %ld seconds after 1969, RTC is predicted to read %ld seconds after 1969.\n"),
>> -			       set_time, set_time + adjustment);
>> +			       set_time, (long)hclocktime.tv_sec);
>> 		}
>> -		display_time(TRUE, set_time + adjustment, -retro);
>> +		display_time(TRUE, hclocktime);
>> 	}
>> 	if (!noadjfile)
>> 		save_adjtime(adjtime, testing);
>> @@ -1646,7 +1618,7 @@ int main(int argc, char **argv)
>> 	/* Variables set by various options; show may also be set later */
>> 	/* The options debug, badyear and epoch_option are global */
>> 	bool show, set, systohc, hctosys, systz, adjust, getepoch, setepoch,
>> -	    predict, compare;
>> +	    predict, compare, get;
>> 	bool utc, testing, local_opt, noadjfile, directisa;
>> 	char *date_opt;
>> #ifdef __alpha__
>> @@ -1659,6 +1631,7 @@ int main(int argc, char **argv)
>> 		OPT_DATE,
>> 		OPT_DIRECTISA,
>> 		OPT_EPOCH,
>> +		OPT_GET,
>> 		OPT_GETEPOCH,
>> 		OPT_LOCALTIME,
>> 		OPT_NOADJFILE,
>> @@ -1706,13 +1679,14 @@ int main(int argc, char **argv)
>> 		{"adjfile",	1, 0, OPT_ADJFILE},
>> 		{"systz",	0, 0, OPT_SYSTZ},
>> 		{"predict-hc",	0, 0, OPT_PREDICT_HC},
>> +		{"get",		0, 0, OPT_GET},
>> 		{NULL,		0, NULL, 0}
>> 	};
>>
>> 	static const ul_excl_t excl[] = {	/* rows and cols in in ASCII order */
>> 		{ 'a','r','s','w',
>> -		  OPT_GETEPOCH,	OPT_PREDICT_HC, OPT_SET,
>> -		  OPT_SETEPOCH, OPT_SYSTZ },
>> +		  OPT_GET, OPT_GETEPOCH, OPT_PREDICT_HC,
>> +		  OPT_SET, OPT_SETEPOCH, OPT_SYSTZ },
>> 		{ 'u', OPT_LOCALTIME},
>> 		{ OPT_ADJFILE, OPT_NOADJFILE },
>> 		{ 0 }
>> @@ -1749,7 +1723,7 @@ int main(int argc, char **argv)
>>
>> 	/* Set option defaults */
>> 	show = set = systohc = hctosys = systz = adjust = noadjfile = predict =
>> -	    compare = FALSE;
>> +	    compare = get = FALSE;
>> 	getepoch = setepoch = utc = local_opt = directisa = testing = debug = FALSE;
>> #ifdef __alpha__
>> 	ARCconsole = Jensen = SRM = funky_toy = badyear = FALSE;
>> @@ -1839,6 +1813,9 @@ int main(int argc, char **argv)
>> 		case OPT_PREDICT_HC:
>> 			predict = TRUE;		/* --predict-hc */
>> 			break;
>> +		case OPT_GET:
>> +			get = TRUE;		/* --get */
>> +			break;
>> #ifdef __linux__
>> 		case 'f':
>> 			rtc_dev_name = optarg;	/* --rtc */
>> @@ -1896,7 +1873,7 @@ int main(int argc, char **argv)
>> 	}
>>
>> 	if (!(show | set | systohc | hctosys | systz | adjust | getepoch
>> -	      | setepoch | predict | compare))
>> +	      | setepoch | predict | compare | get))
>> 		show = 1;	/* default to show */
>>
>> 	if (getuid() == 0)
>> @@ -1953,7 +1930,7 @@ int main(int argc, char **argv)
>> 	} else
>> 		rc = manipulate_clock(show, adjust, noadjfile, set, set_time,
>> 			      hctosys, systohc, systz, startup_time, utc,
>> -			      local_opt, testing, predict);
>> +			      local_opt, testing, predict, get);
>>
>> 	hwclock_exit(rc);
>> 	return rc;		/* Not reached */
>
>-- 
>Sami Kerola
>http://www.iki.fi/kerolasa/
>--
>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 1/7] hwclock: hctosys drift compensation II

[Karel Zak]

On Mon, Sep 29, 2014 at 12:48:06PM -0400, JWP wrote:
> On 09/28/2014 01:55 PM, Sami Kerola wrote:
> >On Sat, 27 Sep 2014, JWP wrote:
> >> +static struct timeval t2tv(time_t timet)
> >
> >More verbose with function name would be easier to understand.
> >
> >time_t_to_timeval(time_t timet)
> >
> 
> I would normally agree, but I think this case is an exception for
> the same reason that all typecasting labels should be short. Imagine
> how ugly the code would look if (int) were (typecast_to_integer). 
> 
> Because time_t and timeval are used extensively throughout this code 
> I think it should be clear to the reader what t2tv() means.
> 
> Having said that, if the project and Mr. Zak believe a longer name is 
> preferred then I will happily go with whatever name is decided upon. 

I have no strong opinion about it, although I usually use _to_ rather
than x2y. 

> >> +{
> >> +  struct timeval rettimeval;
> >> +
> >> +  rettimeval.tv_sec = timet;
> >> +  rettimeval.tv_usec = 0;
> >> +  return(rettimeval);

return is not a function, don't use ().

> >> +}
> >
> >The util-linux is using linux coding style (at least in most of the 
> >tools), so:
> >
> >Tabs are 8 characters, and thus indentations are also 8 characters.
> >
> 
> Well, that's embarrassing.  I thought I had git configured to highlight
> those mistakes for me. My apologizes for such a stupid error. 

OK

> >> +		printf(_("%s  .%06d seconds\n"), ctime_now, (int)hwctime.tv_usec);
> >
> >Why not to avoid cast, and print by using the type the tv_usec is?
> >
> >printf(_("%s  .%06ld seconds\n"), ctime_now, hwctime.tv_usec);
> 
> 
> I was following the existing practice.  All of the print statements
> in this code use typecasting.  I would suggest, rather than breaking
> code continuity by modifying the few instances in this patch, that a

It's fine to follow the existing practice.

> separate patch changing all occurrences would be preferred? I would 
> be happy to add that to my todo list if the project wants it?

Maybe it would be better to think about whole code refactoring as it's
horrible code. It's probably last so horrible code in util-linux.

(refactoring means: add control struct, small smart functions to
 modify the struct and high-level functions to implement hwclock
 logic, but it has to be done in easy to review incremental way. It
 means small patches. 
 
 It's nothing you want to do in this patch-set now. It's another
 (next) task.:-)

> >> +	        struct timeval lastCalib;
> >
> >s/lastCalib/last_calib/
> >
> >CamelCases are unexpected. When fixing that change also 8 spaces in front 
> >of the variable to one tab.
> >
> 
> Very true, more dumb mistakes on my part.

 yes

> Mr. Zak, could you please tell me how to proceed on these changes?
> Should they all be implemented as Mr. Kerola wrote them?
> Do you want to tweak the code, or do you want me to resubmit a 
> single patch, or the entire series?

 You don't have to resend all, just fixed (updated) patches only. Or
 ideally create your own git repository somewhere (e.g. at github) and
 send pull request (url to the repository) only.

 Please, try to compose smaller patches next time, for example --get
 is relatively independent change.

    Karel

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

Re: [PATCH 1/7] hwclock: hctosys drift compensation II

[Sami Kerola]

On 14 October 2014 10:03, Karel Zak <kzak@redhat.com> wrote:
> Maybe it would be better to think about whole code refactoring as it's
> horrible code. It's probably last so horrible code in util-linux.

Along with more(1). I started to look this refactoring task, and it
seems reimplementation would be best option. Does someone fancy a
programming challenge?

Few disk utils, cramfs, bfs, and minix related, could also be improved
or marked deprecated. I am hoping for later.

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

Re: [PATCH 1/7] hwclock: hctosys drift compensation II

[Karel Zak]

On Tue, Oct 14, 2014 at 10:51:30AM +0100, Sami Kerola wrote:
> On 14 October 2014 10:03, Karel Zak <kzak@redhat.com> wrote:
> > Maybe it would be better to think about whole code refactoring as it's
> > horrible code. It's probably last so horrible code in util-linux.
> 
> Along with more(1). I started to look this refactoring task, and it
> seems reimplementation would be best option. Does someone fancy a
> programming challenge?

I prefer refactoring if possible than write things from scratch. And
for hwclock it's really critical to do the changes in small testable
steps.

Note that sfdisk and cfdisk are completely new because we have libfdisk that
replaces all the original cfdisk and sfdisk partitioning code. The
library itself is refactored and incrementally changed fdisk code.

> Few disk utils, cramfs, bfs, and minix related, could also be improved
> or marked deprecated. I am hoping for later.

Well, I don't think we can deprecated these tools and I don't think
that cramfs, minix and bfs code is so bad. For example mkfs.minix
has been clean upped by Davidlohr Bueso years ago. A few global
variables don't not mean that the code is bad...

Compare to hwclock it's like apples and oranges ;-)

    Karel

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

Re: [PATCH 1/7] hwclock: hctosys drift compensation II

[JWP]

On 10/14/2014 05:03 AM, Karel Zak wrote:
> On Mon, Sep 29, 2014 at 12:48:06PM -0400, JWP wrote:
>> On 09/28/2014 01:55 PM, Sami Kerola wrote:
>>> On Sat, 27 Sep 2014, JWP wrote:

>> separate patch changing all occurrences would be preferred? I would 
>> be happy to add that to my todo list if the project wants it?
> 
> Maybe it would be better to think about whole code refactoring as it's
> horrible code. It's probably last so horrible code in util-linux.
> 
> (refactoring means: add control struct, small smart functions to
>  modify the struct and high-level functions to implement hwclock
>  logic, but it has to be done in easy to review incremental way. It
>  means small patches. 

I have added it to my todo list.
I would like to make other needed changes first before starting to
refactor, if that is okay with you?

>> Mr. Zak, could you please tell me how to proceed on these changes?
>> Should they all be implemented as Mr. Kerola wrote them?
>> Do you want to tweak the code, or do you want me to resubmit a 
>> single patch, or the entire series?
> 
>  You don't have to resend all, just fixed (updated) patches only. Or
>  ideally create your own git repository somewhere (e.g. at github) and
>  send pull request (url to the repository) only.

Changes made as requested.

Repository created at GitHub:
https://github.com/jwpi/util-linux
git@github.com:jwpi/util-linux.git
https://github.com/jwpi/util-linux.git

I also sent a pull request to you via GitHub.



> 
>  Please, try to compose smaller patches next time, for example --get
>  is relatively independent change.

Okay, sorry.

> 
>     Karel
> 

Re: [PATCH 1/7] hwclock: hctosys drift compensation II

[Karel Zak]

On Thu, Oct 16, 2014 at 07:21:04PM -0400, JWP wrote:
> Repository created at GitHub:
> https://github.com/jwpi/util-linux
> git@github.com:jwpi/util-linux.git
> https://github.com/jwpi/util-linux.git

 Thanks, merged. I have also added some (I hope correct) notes to
 the begin of the hwclock man page.

    Karel

> 

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

Re: [PATCH 1/7] hwclock: hctosys drift compensation II

[JWP]

On 10/20/2014 08:05 AM, Karel Zak wrote:
> On Thu, Oct 16, 2014 at 07:21:04PM -0400, JWP wrote:
>> Repository created at GitHub:
>> https://github.com/jwpi/util-linux
>> git@github.com:jwpi/util-linux.git
>> https://github.com/jwpi/util-linux.git
> 
>  Thanks, merged. I have also added some (I hope correct) notes to
>  the begin of the hwclock man page.

Hello Karel,

Yes, what you wrote is correct. However, I think some of the wording
could be misinterpreted, especially:

"hwclock automatically compensates Hardware Clock to account for 
 systematic drift before using it to set the System Clock by --hctosys."

Someone may think the Hardware Clock itself was being compensated.

I have made a commit to change the wording slightly, I will send you a
pull request after I send this email.

Thank you for fixing hwclock's man page formatting! It was on my todo list.

Thank you for taking time to review and merge my work, and allowing me to
contribute to util-linux.

I have some util-linux workflow questions:

* Do you want pull requests sent to you via github?
* If yes, what, if anything, should be posted to the mailing list?

Will

Re: [PATCH 1/7] hwclock: hctosys drift compensation II

[Karel Zak]

On Mon, Oct 20, 2014 at 07:35:13PM -0400, JWP wrote:
> Someone may think the Hardware Clock itself was being compensated.
> 
> I have made a commit to change the wording slightly, I will send you a
> pull request after I send this email.

Cool.

> Thank you for taking time to review and merge my work, and allowing me to
> contribute to util-linux.

I hope we will see more your patches ;-)

> * Do you want pull requests sent to you via github?
> * If yes, what, if anything, should be posted to the mailing list?

This mailing list is the core of the util-linux community, the ideal
solution is to send at least the initial version of the patches to the
list for wide discussion and review. The github is nice for repos if
you want to modify just small subset of the patches, or you work on
something large for long time, it's also nice backup solution etc.

I don't like strict rules, just use common sense :-)

(well, except email attachments, it's really painful solution for
 patches, the best is git-send-email)

See also Documentation/howto-contribute.txt and another howtos in the
Documentation/.

    Karel


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

[PATCH 2/7] hwclock: hctosys drift compensation II COMMENTS

[JWP]

Update source comments and --help output for the
hwclock: hctosys drift compensation II patch.

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

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 0dd9ad6..08e79e8 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -683,11 +683,9 @@ set_hardware_clock_exact(const time_t sethwtime,
 }
 
 /*
- * Put the time "systime" on standard output in display format. Except if
+ * 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.
- *
- * Include in the output the adjustment "sync_duration".
  */
 static void
 display_time(const bool hclock_valid, struct timeval hwctime)
@@ -1021,7 +1019,14 @@ adjust_drift_factor(struct adjtime *adjtime_p,
 	        struct timeval lastCalib;
 
 		lastCalib = t2tv(adjtime_p->last_calib_time);
-
+		/*
+		 * Correction to apply to the current drift factor.
+		 *
+		 * Simplified: uncorrected_drift / days_since_calibration.
+		 *
+		 * hclocktime is fully corrected with the current drift factor.
+		 * Its difference from nowtime is the missed drift correction.
+		 */
 		factor_adjust = time_diff(nowtime, hclocktime) /
 				(time_diff(nowtime, lastCalib) / sec_per_day);
 
@@ -1056,16 +1061,12 @@ adjust_drift_factor(struct adjtime *adjtime_p,
 }
 
 /*
- * Do the drift adjustment calculation.
- *
- * The way we have to set the clock, we need the adjustment in two parts:
+ * Calculate the drift correction currently needed for the
+ * Hardware Clock based on the last time it was adjusted,
+ * and the current drift factor, as stored in the adjtime file.
  *
- *	1) an integer number of seconds (return as *adjustment_p)
- *	2) a positive fraction of a second (less than 1) (return as *retro_p)
+ * The total drift adjustment needed is stored at tdrift_p.
  *
- * The sum of these two values is the adjustment needed. Positive means to
- * advance the clock or insert seconds. Negative means to retard the clock
- * or remove seconds.
  */
 static void
 calculate_adjustment(const double factor,
@@ -1160,24 +1161,21 @@ static void save_adjtime(const struct adjtime adjtime, const bool testing)
  * Do not update anything if the Hardware Clock does not currently present a
  * valid time.
  *
- * Arguments <factor> and <last_time> are current values from the adjtime
- * file.
+ * <hclock_valid> means the Hardware Clock contains a valid time.
  *
- * <hclock_valid> means the Hardware Clock contains a valid time, and that
- * time is <hclocktime>.
+ * <hclocktime> is the drift corrected time read from the Hardware Clock.
  *
- * <read_time> is the current system time (to be precise, it is the system
- * time at the time <hclocktime> was read, which due to computational delay
- * could be a short time ago).
+ * <read_time> was the system time when the <hclocktime> was read, which due
+ * to computational delay could be a short time ago. It is used to define a
+ * trigger point for setting the Hardware Clock. The fractional part of the
+ * Hardware clock set time is subtracted from read_time to 'refer back', or
+ * delay, the trigger point.  Fractional parts must be accounted for in this
+ * way, because the Hardware Clock can only be set to a whole second.
  *
  * <universal>: the Hardware Clock is kept in UTC.
  *
  * <testing>:  We are running in test mode (no updating of clock).
  *
- * We do not bother to update the clock if the adjustment would be less than
- * one second. This is to avoid cumulative error and needless CPU hogging
- * (remember we use an infinite loop for some timing) if the user runs us
- * frequently.
  */
 static void
 do_adjustment(struct adjtime *adjtime_p,
@@ -1265,11 +1263,11 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
 	 */
 	bool hclock_valid = FALSE;
 	/*
-	 * The time the hardware clock had just after we
-	 * synchronized to its next clock tick when we
-	 * started up. Defined only if hclock_valid is true.
+	 * Tick synchronized time read from the Hardware Clock and
+	 * then drift correct for all operations except --show.
 	 */
 	struct timeval hclocktime = { 0, 0 };
+	/* Total Hardware Clock drift correction needed. */
 	struct timeval tdrift;
 	/* local return code */
 	int rc = 0;
@@ -1325,6 +1323,14 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
 				return EX_IOERR;
 		}
 	}
+	/*
+	 * 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 = predict ? t2tv(set_time) : hclocktime;
 	calculate_adjustment(adjtime.drift_factor,
 			     adjtime.last_adj_time,
@@ -1544,6 +1550,7 @@ static void usage(const char *fmt, ...)
 	fputs(_("\nFunctions:\n"), usageto);
 	fputs(_(" -h, --help           show this help text and exit\n"
 		" -r, --show           read hardware clock and print result\n"
+		"     --get            read hardware clock and print drift corrected result\n"
 		"     --set            set the RTC to the time given with --date\n"), usageto);
 	fputs(_(" -s, --hctosys        set the system time from the hardware clock\n"
 		" -w, --systohc        set the hardware clock from the current system time\n"

[PATCH 3/7] hwclock: hctosys drift compensation II MAN

[JWP]

Update hwclock man page for the
hwclock: hctosys drift compensation II patch.

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

diff --git a/sys-utils/hwclock.8.in b/sys-utils/hwclock.8.in
index b11b45c..913da37 100644
--- a/sys-utils/hwclock.8.in
+++ b/sys-utils/hwclock.8.in
@@ -61,8 +61,18 @@ in Coordinated Universal Time.  See the
 option.
 Showing the Hardware Clock time is the default when no function is specified.
 .TP
+.B \-\-get
+Like
+.B --show
+only with drift correction applied to the time read. This is useful when the
+Hardware Clock is not being periodically updated by something such as NTP's
+11 minute mode or when not using
+.BR --adjust .
+.TP
 .BR \-s , \ \-\-hctosys
-Set the System Time from the Hardware Clock.
+Set the System Time from the Hardware Clock.  The time read from the Hardware
+Clock is compensated to account for systematic drift before using it to set the
+System Clock.  See the discussion below, under \fBThe Adjust Function\fR.
 .PP
 Also set the kernel's timezone value to the local timezone
 as indicated by the TZ environment variable and/or
@@ -484,8 +494,9 @@ The Hardware Clock is usually not very accurate.  However, much of its
 inaccuracy is completely predictable - it gains or loses the same amount
 of time every day.  This is called systematic drift.
 .BR hwclock 's
-"adjust" function lets you make systematic corrections to correct the
-systematic drift.
+.I \-\-adjust
+function lets you apply systematic drift corrections to the
+Hardware Clock.
 .PP
 It works like this:
 .B hwclock
@@ -529,20 +540,25 @@ since the last calibration, how long it has been since the last
 adjustment, what drift rate was assumed in any intervening
 adjustments, and the amount by which the clock is presently off.
 .PP
-A small amount of error creeps in any time
-.B hwclock
-sets the clock, so it refrains from making an adjustment that would be
-less than 1 second.  Later on, when you request an adjustment again,
-the accumulated drift will be more than a second and
-.B hwclock
-will do the adjustment then.
-.PP
-It is good to do a
-.I hwclock \-\-adjust
-just before the
-.I hwclock \-\-hctosys
-at system startup time, and maybe periodically while the system is
-running via cron.
+A small amount of error creeps in when
+the Hardware Clock is set, so
+.I \-\-adjust
+refrains from making any adjustment that is less
+than 1 second.  Later on, when you request an adjustment again, the accumulated
+drift will be more than 1 second and
+.I \-\-adjust
+will make the adjustment including any fractional amount.
+.PP
+.IR "hwclock \-\-hctosys"
+also uses the adjtime file data to compensate the value read from the Hardware
+Clock before using it to set the System Time.  It does not share the 1 second
+limitation of --adjust, and will correct sub-second drift values immediately.
+It does not change the Hardware Clock time or the adjtime file.  This may
+eliminate the need to use --adjust, unless something else on the system needs
+the Hardware Clock to be compensated. The drift compensation can be inhibited
+by using the
+.B --noadjfile
+option.
 .PP
 The adjtime file, while named for its historical purpose of controlling
 adjustments only, actually contains other information for use by hwclock

[PATCH 4/7] hwclock: persistent_clock_is_local

[JWP]

When hctosys is used at boot time, making it the
first caller of settimeofday, the responsibility
of setting persistent_clock_is_local is thrust
upon it.  Currently hctosys always leaves this
variable uninitialized.  This causes a Hardware
Clock configured to use the local timescale to be
clobbered with the UTC timescale by the kernel's
NTP eleven minute mode.

This patch fixes this hctosys bug, by having it
properly set persistent_clock_is_local according
to the time scale configured for the Hardware
Clock.

It does this via the kernel warp_clock function
but this in inconsequential, because we set the
system time immediately afterward.

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

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 08e79e8..42f54c2 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -808,6 +808,12 @@ static int interpret_date_string(const char *date_opt, time_t * const time_p)
  * environment variable and/or /usr/lib/zoneinfo/, interpreted as tzset()
  * would interpret them.
  *
+ * If this is the first call of settimeofday since boot, then this also sets
+ * the kernel variable persistent_clock_is_local so that NTP 11 minute mode
+ * will update the Hardware Clock with the proper timescale. If the Hardware
+ * 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.
@@ -817,7 +823,7 @@ static int interpret_date_string(const char *date_opt, time_t * const time_p)
  */
 static int
 set_system_clock(const bool hclock_valid, const struct timeval newtime,
-		 const bool testing)
+		 const bool testing, const bool universal)
 {
 	int retcode;
 
@@ -827,9 +833,10 @@ set_system_clock(const bool hclock_valid, const struct timeval newtime,
 		       "we cannot set the System Time from it."));
 		retcode = 1;
 	} else {
+		const struct timeval *tv_null = NULL;
 		struct tm *broken;
 		int minuteswest;
-		int rc;
+		int rc = 0;
 
 		broken = localtime(&newtime.tv_sec);
 #ifdef HAVE_TM_GMTOFF
@@ -853,7 +860,14 @@ set_system_clock(const bool hclock_valid, const struct timeval newtime,
 		} else {
 			const struct timezone tz = { minuteswest, 0 };
 
-			rc = settimeofday(&newtime, &tz);
+			/* 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 (!universal)
+				rc = settimeofday(tv_null, &tz);
+			if (!rc)
+				rc = settimeofday(&newtime, &tz);
 			if (rc) {
 				if (errno == EPERM) {
 					warnx(_
@@ -1378,7 +1392,8 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
 			adjust_drift_factor(&adjtime, nowtime,
 					    hclock_valid, hclocktime);
 	} else if (hctosys) {
-		rc = set_system_clock(hclock_valid, hclocktime, testing);
+		rc = set_system_clock(hclock_valid, hclocktime,
+				      testing, universal);
 		if (rc) {
 			printf(_("Unable to set system clock.\n"));
 			return rc;

[PATCH 5/7] hwclock: persistent_clock_is_local MAN

[JWP]

Update hwclock man page for the
hwclock: persistent_clock_is_local patch.

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

diff --git a/sys-utils/hwclock.8.in b/sys-utils/hwclock.8.in
index 913da37..d04a429 100644
--- a/sys-utils/hwclock.8.in
+++ b/sys-utils/hwclock.8.in
@@ -84,7 +84,24 @@ The obsolete tz_dsttime field of the kernel's timezone value is set
 to DST_NONE.  (For details on what this field used to mean, see
 .BR settimeofday (2).)
 .PP
+When used in a startup script, making it the first caller of
+.BR settimeofday (2)
+from boot, it will set the NTP 11 minute mode time scale via the
+.I persistent_clock_is_local
+kernel variable. See the discussion below, under
+.B Automatic Hardware Clock Synchronization by the
+.BR Kernel.
+.PP
 This is a good option to use in one of the system startup scripts.
+.PP
+This option should never be used on a running system. Jumping system time
+will cause problems, such as, corrupted file system timestamps.
+Also, if NTP 11 minute mode is active then
+.B --hctosys
+will set the time incorrectly by
+including drift compensation. Drift compensation can be inhibited by using the
+.B --noadjfile
+option.
 .TP
 .B \-\-set
 Set the Hardware Clock to the time given by the
@@ -393,11 +410,16 @@ use the TZ environment variable and/or the
 .I /usr/share/zoneinfo
 directory, as explained in the man page for
 .BR tzset (3).
-However, some
-programs and fringe parts of the Linux kernel such as filesystems use
-the kernel timezone value.  An example is the vfat filesystem.  If the
-kernel timezone value is wrong, the vfat filesystem will report and
-set the wrong timestamps on files.
+However, some programs and fringe parts of the Linux kernel such as filesystems
+use the kernel timezone value.  An example is the vfat filesystem.  If the
+kernel timezone value is wrong, the vfat filesystem will report and set the
+wrong timestamps on files. Another example is the kernel's NTP 11 minute mode.
+If the kernel's timezone value and/or the
+.I persistent_clock_is_local
+variable are wrong, then the Hardware Clock will be set incorrectly by 11 minute
+mode.  See the discussion below, under
+.B Automatic Hardware Clock Synchronization by the
+.BR Kernel.
 .PP
 .B hwclock
 sets the kernel timezone to the value indicated by TZ and/or
@@ -601,20 +623,31 @@ your System Time synchronized either to a time server somewhere on the
 network or to a radio clock hooked up to your system.  See RFC 1305.)
 .PP
 This mode (we'll call it "11 minute mode") is off until something
-turns it on.  The ntp daemon xntpd is one thing that turns it on.  You
+turns it on.  The ntp daemon ntpd is one thing that turns it on.  You
 can turn it off by running anything, including
 .IR "hwclock \-\-hctosys" ,
-that sets the System Time the old fashioned way.
+that sets the System Time the old fashioned way.  However, if the ntp daemon is
+still running, it will turn 11 minute mode back on again the next time it
+synchronizes the System Clock.
 .PP
-If your system runs with 11 minute mode on, don't use
-.I hwclock \-\-adjust
-or
-.IR "hwclock \-\-hctosys" .
-You'll just make a mess.  It is acceptable to use a
+If your system runs with 11 minute mode on, it may need
+.I hwclock \-\-hctosys
+in a startup script, especially if the Hardware Clock is configured to to use
+the local timescale.
+
+The first user space command to set the System Clock informs the
+kernel what timescale the Hardware Clock is using. This happens via the
+.I persistent_clock_is_local
+kernel variable. If
 .I hwclock \-\-hctosys
-at startup time to get a reasonable System Time until your system is
-able to set the System Time from the external source and start 11
-minute mode.
+is the first, it will set this variable according to the adjtime file or the
+appropriate command line argument. Note that when using this capability and the
+Hardware Clock timescale configuration is changed, then a reboot is required to
+notify the kernel.
+
+Don't use
+.I hwclock \-\-adjust
+with 11 minute mode.  You'll just make a mess.
 
 .SS ISA Hardware Clock Century value
 .PP

[PATCH 6/7] hwclock: Add --update option

[JWP]

There are cases where we need to refresh the
timestamps in the adjtime file without updating the
drift factor.

For example, with ntpd and an Eleven Minute Mode
kernel, we need to call systohc at shutdown to
facilitate drift correction.  With the current
behavior hwclock will clobber the drift factor to
near zero, because the Hardware Clock and System
Clock are synced by Eleven Minute Mode.  What
actually needs to be done is refresh the adjtime
file timestamps and not calculate a new drift
factor.

Because it is a manual process to craft a good
Hardware Clock drift factor, that is, there is no
automated method that will produce a good drift
factor, this patch changes the default drift
calculation behavior to off, and it is turned on
by using the --update option. Once we have a good
drift factor for a given machine we do not want
anything clobbering it, including an administrator
forgetting to turn off recalculation. A system
administrator should make a concious effort in
telling hwclock with the --update option that
(s)he wants to recalculate the drift factor.

Without using the --update option with calibrate
operations only the timestamps are refreshed in
the adjtime file. With the --update option the old
default behavior of refreshing the timestamps and
updating the drift factor is performed.

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

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 42f54c2..c5c3560 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -980,12 +980,13 @@ static int set_system_clock_timezone(const bool universal, const bool testing)
 }
 
 /*
- * Update the drift factor in <*adjtime_p> to reflect the fact that the
- * Hardware Clock was calibrated to <nowtime> and before that was set to
- * <hclocktime>.
+ * Refresh the last calibrated and last adjusted timestamps in <*adjtime_p>
+ * to facilitate future drift calculations based on this set point.
  *
- * We record in the adjtime file the time at which we last calibrated the
- * clock so we can compute the drift rate each time we calibrate.
+ * With the --update option:
+ * 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,
@@ -995,9 +996,20 @@ static void
 adjust_drift_factor(struct adjtime *adjtime_p,
 		    const struct timeval nowtime,
 		    const bool hclock_valid,
-		    const struct timeval hclocktime)
+		    const struct timeval hclocktime,
+		    const bool update)
 {
-	if (!hclock_valid) {
+	if (!update) {
+		/*
+		 * Because the update option introduced new behavior to hwclock
+		 * we warn when it is not used. After a reasonable introduction
+		 * period this could be removed.
+		 */
+		warnx(_("--update option is now required to update drift factor."));
+		if (debug)
+			printf(_("Not adjusting drift factor because the "
+				 "--update option was not used.\n"));
+	} else if (!hclock_valid) {
 		if (debug)
 			printf(_("Not adjusting drift factor because the "
 				 "Hardware Clock previously contained "
@@ -1015,14 +1027,15 @@ adjust_drift_factor(struct adjtime *adjtime_p,
 				 "calibration.\n"));
 	} else if (adjtime_p->last_calib_time != 0) {
 		/*
-		 * At adjustment time we adjust the hardware clock according
-		 * to the contents of /etc/adjtime.
+		 * At adjustment time we drift correct the hardware clock
+		 * according to the contents of the adjtime file and refresh
+		 * its last adjusted timestamp.
 		 *
-		 * At calibration time we set the hardware clock and update
-		 * /etc/adjtime, that is, for each calibration (except the
-		 * first) we also do an adjustment.
+		 * At calibration time we set the Hardware Clock and refresh
+		 * both timestamps in <*adjtime_p>.
 		 *
-		 * We are now at calibration time.
+		 * Here, with the --update option, we also update the drift
+		 * factor in <*adjtime_p>.
 		 *
 		 * Let us do computation in doubles. (Floats almost suffice,
 		 * but 195 days + 1 second equals 195 days in floats.)
@@ -1261,7 +1274,7 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
 		 const bool set, const time_t set_time,
 		 const bool hctosys, const bool systohc, const bool systz,
 		 const struct timeval startup_time,
-		 const bool utc, const bool local_opt,
+		 const bool utc, const bool local_opt, const bool update,
 		 const bool testing, const bool predict, const bool get)
 {
 	/* Contents of the adjtime file, or what they should be. */
@@ -1366,7 +1379,7 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
 			adjust_drift_factor(&adjtime,
 					    time_inc(t2tv(set_time), time_diff
 						     (read_time, startup_time)),
-					    hclock_valid, hclocktime);
+					    hclock_valid, hclocktime, update);
 	} else if (adjust) {
 		if (tdrift.tv_sec > 0 || tdrift.tv_sec < -1)
 			do_adjustment(&adjtime, hclock_valid,
@@ -1390,7 +1403,7 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
 					 reftime, universal, testing);
 		if (!noadjfile)
 			adjust_drift_factor(&adjtime, nowtime,
-					    hclock_valid, hclocktime);
+					    hclock_valid, hclocktime, update);
 	} else if (hctosys) {
 		rc = set_system_clock(hclock_valid, hclocktime,
 				      testing, universal);
@@ -1594,10 +1607,11 @@ static void usage(const char *fmt, ...)
 		"     --epoch <year>   specifies the year which is the beginning of the\n"
 		"                        hardware clock's epoch value\n"), _PATH_RTC_DEV);
 	fprintf(usageto, _(
+		"     --update         update drift factor in %s\n"
 		"     --noadjfile      do not access %s; this requires the use of\n"
 		"                        either --utc or --localtime\n"
 		"     --adjfile <file> specifies the path to the adjust file;\n"
-		"                        the default is %s\n"), _PATH_ADJTIME, _PATH_ADJTIME);
+		"                        the default is %s\n"), _PATH_ADJTIME, _PATH_ADJTIME, _PATH_ADJTIME);
 	fputs(_("     --test           do not update anything, just show what would happen\n"
 		" -D, --debug          debugging mode\n" "\n"), usageto);
 #ifdef __alpha__
@@ -1641,7 +1655,7 @@ int main(int argc, char **argv)
 	/* The options debug, badyear and epoch_option are global */
 	bool show, set, systohc, hctosys, systz, adjust, getepoch, setepoch,
 	    predict, compare, get;
-	bool utc, testing, local_opt, noadjfile, directisa;
+	bool utc, testing, local_opt, update, noadjfile, directisa;
 	char *date_opt;
 #ifdef __alpha__
 	bool ARCconsole, Jensen, SRM, funky_toy;
@@ -1661,7 +1675,8 @@ int main(int argc, char **argv)
 		OPT_SET,
 		OPT_SETEPOCH,
 		OPT_SYSTZ,
-		OPT_TEST
+		OPT_TEST,
+		OPT_UPDATE
 	};
 
 	static const struct option longopts[] = {
@@ -1702,6 +1717,7 @@ int main(int argc, char **argv)
 		{"systz",	0, 0, OPT_SYSTZ},
 		{"predict-hc",	0, 0, OPT_PREDICT_HC},
 		{"get",		0, 0, OPT_GET},
+		{"update",      0, 0, OPT_UPDATE},
 		{NULL,		0, NULL, 0}
 	};
 
@@ -1711,6 +1727,7 @@ int main(int argc, char **argv)
 		  OPT_SET, OPT_SETEPOCH, OPT_SYSTZ },
 		{ 'u', OPT_LOCALTIME},
 		{ OPT_ADJFILE, OPT_NOADJFILE },
+		{ OPT_NOADJFILE, OPT_UPDATE },
 		{ 0 }
 	};
 	int excl_st[ARRAY_SIZE(excl)] = UL_EXCL_STATUS_INIT;
@@ -1745,7 +1762,7 @@ int main(int argc, char **argv)
 
 	/* Set option defaults */
 	show = set = systohc = hctosys = systz = adjust = noadjfile = predict =
-	    compare = get = FALSE;
+	    compare = get = update = FALSE;
 	getepoch = setepoch = utc = local_opt = directisa = testing = debug = FALSE;
 #ifdef __alpha__
 	ARCconsole = Jensen = SRM = funky_toy = badyear = FALSE;
@@ -1838,6 +1855,9 @@ int main(int argc, char **argv)
 		case OPT_GET:
 			get = TRUE;		/* --get */
 			break;
+		case OPT_UPDATE:
+			update = TRUE;		/* --update */
+			break;
 #ifdef __linux__
 		case 'f':
 			rtc_dev_name = optarg;	/* --rtc */
@@ -1952,7 +1972,7 @@ int main(int argc, char **argv)
 	} else
 		rc = manipulate_clock(show, adjust, noadjfile, set, set_time,
 			      hctosys, systohc, systz, startup_time, utc,
-			      local_opt, testing, predict, get);
+			      local_opt, update, testing, predict, get);
 
 	hwclock_exit(rc);
 	return rc;		/* Not reached */

Re: [PATCH 6/7] hwclock: Add --update option

[Karel Zak]

On Sat, Sep 27, 2014 at 11:30:01AM -0400, JWP wrote:
> 
> There are cases where we need to refresh the
> timestamps in the adjtime file without updating the
> drift factor.
> 
> For example, with ntpd and an Eleven Minute Mode
> kernel, we need to call systohc at shutdown to
> facilitate drift correction.  With the current
> behavior hwclock will clobber the drift factor to
> near zero, because the Hardware Clock and System
> Clock are synced by Eleven Minute Mode.  What
> actually needs to be done is refresh the adjtime
> file timestamps and not calculate a new drift
> factor.
> 
> Because it is a manual process to craft a good
> Hardware Clock drift factor, that is, there is no
> automated method that will produce a good drift
> factor, this patch changes the default drift

The ideal solution would be to generate event (for udev) in kernel
first time when Eleven Minute Mode modifies HW clock and send info
about the diff between HW and SYS time to userspace. Then it would be
possible to call something like "hwclock --set-drift <number>" from udev
rules. It means 11 minutes after reboot we will have nice usable
drift factor for the next system startup (--hctosys).

> calculation behavior to off, and it is turned on
> by using the --update option. Once we have a good
> drift factor for a given machine we do not want
> anything clobbering it, including an administrator
> forgetting to turn off recalculation. A system
> administrator should make a concious effort in
> telling hwclock with the --update option that
> (s)he wants to recalculate the drift factor.
> Without using the --update option with calibrate
> operations only the timestamps are refreshed in
> the adjtime file. With the --update option the old
> default behavior of refreshing the timestamps and
> updating the drift factor is performed.

The option "--update" seems to generic. It would be better
to use something like "--update-drift".

> +	if (!update) {
> +		/*
> +		 * Because the update option introduced new behavior to hwclock
> +		 * we warn when it is not used. After a reasonable introduction
> +		 * period this could be removed.
> +		 */
> +		warnx(_("--update option is now required to update drift factor."));

I'd like to see something like

   warnx(_("HW clock drift factor not updated by default. "
           "See --update-drift for more details."));

and maybe it should be visible in verbose mode only. All we need is to
inform distro maintainers by ReleaseNotes rather than fill logs by
confusing messages.

>  		rc = manipulate_clock(show, adjust, noadjfile, set, set_time,
>  			      hctosys, systohc, systz, startup_time, utc,
> -			      local_opt, testing, predict, get);
> +			      local_opt, update, testing, predict, get);

 This is why we need refactoring. 15 arguments to function? Grrr..

    Karel

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

[PATCH 7/7] hwclock: Add --update option MAN

[JWP]

Update hwclock man page for the
hwclock: Add --update option patch.

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

diff --git a/sys-utils/hwclock.8.in b/sys-utils/hwclock.8.in
index d04a429..1948251 100644
--- a/sys-utils/hwclock.8.in
+++ b/sys-utils/hwclock.8.in
@@ -342,7 +342,18 @@ with SRM console.
 Do everything except actually updating the Hardware Clock or anything
 else.  This is useful, especially in conjunction with
 .BR \-\-debug ,
-in learning about
+in learning about the internal operations of hwclock.
+
+.TP
+.B \-\-update
+Update the Hardware Clock's drift factor in
+.IR @ADJTIME_PATH@ .
+It is used with
+.BR --set\  or \ --systohc ,
+otherwise it is ignored.
+See the discussion below, under
+.B The Adjust
+.BR Function.
 
 .TP
 .BR \-u , \ \-\-utc
@@ -532,8 +543,8 @@ command to set the Hardware Clock to the true current time.
 .B hwclock
 creates the adjtime file and records in it the current time as the
 last time the clock was calibrated.
-5 days later, the clock has gained 10 seconds, so you issue another
-.I hwclock \-\-set
+5 days later, the clock has gained 10 seconds, so you issue the
+.I hwclock \-\-set \-\-update
 command to set it back 10 seconds.
 .B hwclock
 updates the adjtime file to show the current time as the last time the
@@ -552,15 +563,16 @@ Another 24 hours goes by and you issue another
 does the same thing: subtracts 2 seconds and updates the adjtime file
 with the current time as the last time the clock was adjusted.
 .PP
-Every time you calibrate (set) the clock (using
-.I \-\-set
-or
-.IR \-\-systohc ),
-.B hwclock
-recalculates the systematic drift rate based on how long it has been
+When you use the
+.I \-\-update
+option with
+.IR \-\-set\  or \ \-\-systohc ,
+the systematic drift rate is (re)calculated based on how long it has been
 since the last calibration, how long it has been since the last
 adjustment, what drift rate was assumed in any intervening
-adjustments, and the amount by which the clock is presently off.
+adjustments, and the amount by which the clock is presently off.  This updated
+drift factor is then saved in
+.IR @ADJTIME_PATH@ .
 .PP
 A small amount of error creeps in when
 the Hardware Clock is set, so