hwclock patch cover letter.

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 <[email protected]>

---
sys-utils/hwclock.c | 205 +++++++++++++++++++++++-----------------------------

1 file changed, 91 insertions(+), 114 deletions(-)

sys-utils/hwclock.c +0 -0

index 474e04f..0dd9ad6 100644

sys-utils/hwclock.c +1 -0

+/*
 * 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

sys-utils/hwclock.c +1 -2

-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(_

sys-utils/hwclock.c +0 -0

 	}
 }
 

@@ -807,7 +818,7 @@ static int interpret_date_string(const char *date_opt, time_t * const time_p)
 * have.
 */

static int

sys-utils/hwclock.c +1 -1

-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;

sys-utils/hwclock.c +1 -5

@@ -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

sys-utils/hwclock.c +1 -1

 			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,

sys-utils/hwclock.c +1 -1

-		    const time_t nowtime,
+		    const struct timeval nowtime,

const bool hclock_valid,

sys-utils/hwclock.c +1 -1

-		    const time_t hclocktime, const double sync_delay)
+		    const struct timeval hclocktime)

{
	if (!hclock_valid) {
		if (debug)

sys-utils/hwclock.c +1 -1

@@ -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 "

sys-utils/hwclock.c +14 -38

-		/* 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,

sys-utils/hwclock.c +1 -1

-	      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)
{

sys-utils/hwclock.c +53 -57

@@ -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 */

Quoted 51 lines · 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 <[email protected]>

---
sys-utils/hwclock.c | 205 +++++++++++++++++++++++-----------------------------

1 file changed, 91 insertions(+), 114 deletions(-)

sys-utils/hwclock.c +0 -0

index 474e04f..0dd9ad6 100644
@@ -181,6 +181,18 @@ static void read_date_from_file(struct tm *tm)
 }
 

sys-utils/hwclock.c static void read_date_from_file(struct tm *tm) · line 186

Quoted 4 lines

sys-utils/hwclock.c +3 -0

 /*
+ * 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)

sys-utils/hwclock.c static void read_date_from_file(struct tm *tm) · line 193

Quoted 7 lines

sys-utils/hwclock.c +7 -0

+{
+  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.

Quoted 15 lines

sys-utils/hwclock.c +1 -0

+/*
 * 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

sys-utils/hwclock.c +1 -2

-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(_

sys-utils/hwclock.c display_time(const bool hclock_valid, const time_t systime, · line 707

Quoted 8 lines

sys-utils/hwclock.c +2 -2

@@ -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);

Quoted 21 lines

sys-utils/hwclock.c +0 -0

 	}
 }
 

@@ -807,7 +818,7 @@ static int interpret_date_string(const char *date_opt, time_t * const time_p)
 * have.
 */

static int

sys-utils/hwclock.c +1 -1

-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;

sys-utils/hwclock.c +0 -1

@@ -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;
 

Quoted 9 lines

sys-utils/hwclock.c +1 -4

-		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

sys-utils/hwclock.c set_system_clock(const bool hclock_valid, const time_t newtime, · line 848

Quoted 5 lines

sys-utils/hwclock.c +1 -1

@@ -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

Quoted 35 lines

sys-utils/hwclock.c +1 -1

 			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,

sys-utils/hwclock.c +1 -1

-		    const time_t nowtime,
+		    const struct timeval nowtime,

const bool hclock_valid,

sys-utils/hwclock.c +1 -1

-		    const time_t hclocktime, const double sync_delay)
+		    const struct timeval hclocktime)

{
	if (!hclock_valid) {
		if (debug)

sys-utils/hwclock.c +1 -1

@@ -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 "

sys-utils/hwclock.c adjust_drift_factor(struct adjtime *adjtime_p, · line 1021

Quoted 9 lines

sys-utils/hwclock.c +1 -3

@@ -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.

Quoted 338 lines

sys-utils/hwclock.c +14 -38

-		/* 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,

sys-utils/hwclock.c +1 -1

-	      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)
{

sys-utils/hwclock.c +53 -57

@@ -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 */

Signature · 6 lines

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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 7 lines · Sami Kerola

Quoted 1 line · JWP

+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.

Quoted 6 lines

sys-utils/hwclock.c static void read_date_from_file(struct tm *tm) · line 192

Quoted 6 lines

sys-utils/hwclock.c +6 -0

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

Quoted 7 lines

Quoted 1 line

+}

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.

Quoted 5 lines

sys-utils/hwclock.c display_time(const bool hclock_valid, const time_t systime, · line 707

Quoted 1 line

sys-utils/hwclock.c +1 -0

+		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?

Quoted 7 lines

sys-utils/hwclock.c adjust_drift_factor(struct adjtime *adjtime_p, · line 1021

Quoted 1 line

sys-utils/hwclock.c +1 -0

+	        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?

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.

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.

Quoted 2 lines · Karel Zak

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.

Quoted 5 lines · JWP

Quoted 1 line · Sami Kerola

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.

Quoted 4 lines

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

Repository created at GitHub:
https://github.com/jwpi/util-linux
[email protected]:jwpi/util-linux.git
https://github.com/jwpi/util-linux.git

Quoted 4 lines · JWP

Repository created at GitHub:
https://github.com/jwpi/util-linux
[email protected]: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.

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 taking time to review and merge my work, and allowing me to contribute to util-linux.

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