[PATCH 0/2] Pull Request

[PATCH 0/2] Pull Request

[J William Piggott]

The following changes since commit 43afa84581de8984aa00ef2e9208198929f72ddf:

  lib/mbsalign: encode \x to \xecx (2017-12-20 13:01:43 +0100)

are available in the git repository at:

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

for you to fetch changes up to e3fac9e420c55ae804199da6be2eb08ba9788795:

  hwclock: add --ul-debug implementing debug.h (2017-12-24 14:19:26 -0500)

----------------------------------------------------------------
J William Piggott (2):
      hwclock: rename --debug option to --verbose
      hwclock: add --ul-debug implementing debug.h

 Documentation/deprecated.txt |  12 +++++
 sys-utils/hwclock-rtc.c      |  10 ++---
 sys-utils/hwclock.8.in       |  15 ++++---
 sys-utils/hwclock.c          | 105 ++++++++++++++++++++++++-------------------
 sys-utils/hwclock.h          |  14 ++++--
 5 files changed, 96 insertions(+), 60 deletions(-)

[PATCH 1/2] hwclock: rename --debug option to --verbose

[J William Piggott]

Warn on --debug; do not fallthrough because
the message is lost in the verbose output.

Coauthored-by: Sami Kerola <kerolasa@iki.fi>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
Signed-off-by: J William Piggott <elseifthen@gmx.com>
---
 Documentation/deprecated.txt | 12 ++++++++
 sys-utils/hwclock-rtc.c      | 10 +++---
 sys-utils/hwclock.8.in       | 15 ++++++---
 sys-utils/hwclock.c          | 73 +++++++++++++++++++++++---------------------
 sys-utils/hwclock.h          |  3 +-
 5 files changed, 66 insertions(+), 47 deletions(-)

diff --git a/Documentation/deprecated.txt b/Documentation/deprecated.txt
index 34ea698da..f1ec8953d 100644
--- a/Documentation/deprecated.txt
+++ b/Documentation/deprecated.txt
@@ -2,6 +2,18 @@ The following is a list of commands or features that are deprecated.  All
 deprecated utils are in maintenance mode and we keep them in source tree for
 backward compatibility only.
 
+what:  hwclock --debug
+why:   renamed to --verbose, and may be repurposed later.
+since: v2.32
+
+--------------------------
+
+what:  hwclock -v for version
+why:   renamed to -V
+since: v2.20 (was repurposed to verbose in v2.32)
+
+--------------------------
+
 what:  column --columns
 why:   renamed to --output-width
 since: v2.30
diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
index cdd7d80d6..ef95d9807 100644
--- a/sys-utils/hwclock-rtc.c
+++ b/sys-utils/hwclock-rtc.c
@@ -124,7 +124,7 @@ static int open_rtc(const struct hwclock_control *ctl)
 		rtc_dev_fd = open(rtc_dev_name, O_RDONLY);
 	} else {
 		for (i = 0; i < ARRAY_SIZE(fls); i++) {
-			if (ctl->debug)
+			if (ctl->verbose)
 				printf(_("Trying to open: %s\n"), fls[i]);
 			rtc_dev_fd = open(fls[i], O_RDONLY);
 
@@ -208,7 +208,7 @@ static int busywait_for_rtc_clock_tick(const struct hwclock_control *ctl,
 	int rc;
 	struct timeval begin, now;
 
-	if (ctl->debug) {
+	if (ctl->verbose) {
 		printf("ioctl(%d, RTC_UIE_ON, 0): %s\n",
 		       rtc_fd, strerror(errno));
 		printf(_("Waiting in loop for time from %s to change\n"),
@@ -358,7 +358,7 @@ static int set_hardware_clock_rtc(const struct hwclock_control *ctl,
 		hwclock_exit(ctl, EXIT_FAILURE);
 	}
 
-	if (ctl->debug)
+	if (ctl->verbose)
 		printf(_("ioctl(%s) was successful.\n"), ioctlname);
 
 	return 0;
@@ -407,7 +407,7 @@ int get_epoch_rtc(const struct hwclock_control *ctl, unsigned long *epoch_p)
 		return 1;
 	}
 
-	if (ctl->debug)
+	if (ctl->verbose)
 		printf(_("ioctl(%d, RTC_EPOCH_READ, epoch_p) to %s succeeded.\n"),
 		       rtc_fd, rtc_dev_name);
 
@@ -442,7 +442,7 @@ int set_epoch_rtc(const struct hwclock_control *ctl)
 		return 1;
 	}
 
-	if (ctl->debug)
+	if (ctl->verbose)
 		printf(_("ioctl(%d, RTC_EPOCH_SET, %lu) to %s succeeded.\n"),
 		       rtc_fd, epoch, rtc_dev_name);
 
diff --git a/sys-utils/hwclock.8.in b/sys-utils/hwclock.8.in
index d88feb07b..b9f618973 100644
--- a/sys-utils/hwclock.8.in
+++ b/sys-utils/hwclock.8.in
@@ -281,10 +281,9 @@ parameters should be observed.
 .
 .TP
 .BR \-D ", " \-\-debug
-Display a lot of information about what
-.B \%hwclock
-is doing internally.  Some of its functions are complex and this output
-can help you understand how the program works.
+.RB Use\  \-\-verbose .
+.RB The\  \%\-\-debug\  option
+has been deprecated and may be repurposed or removed in a future release.
 .
 .TP
 .B \-\-directisa
@@ -374,7 +373,7 @@ must be specified when using this option.
 .B \-\-test
 Do not actually change anything on the system, that is, the Clocks or
 .I @ADJTIME_PATH@
-.RB ( \%\-\-debug
+.RB ( \%\-\-verbose
 is implicit with this option).
 .
 .TP
@@ -430,6 +429,12 @@ option.  Despite it not working, the resulting drift correction factor
 would be invalid anyway.
 .RE
 .
+.TP
+.BR \-v ", " \-\-verbose
+Display more details about what
+.B \%hwclock
+is doing internally.
+.
 .SH NOTES
 .
 .SS Clocks in a Linux System
diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 71d2b17b7..0dc1d2369 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -179,7 +179,7 @@ hw_clock_is_utc(const struct hwclock_control *ctl,
 	else
 		/* get info from adjtime file - default is UTC */
 		ret = (adjtime.local_utc != LOCAL);
-	if (ctl->debug)
+	if (ctl->verbose)
 		printf(_("Assuming hardware clock is kept in %s time.\n"),
 		       ret ? _("UTC") : _("local"));
 	return ret;
@@ -236,7 +236,7 @@ static int read_adjtime(const struct hwclock_control *ctl,
 		}
 	}
 
-	if (ctl->debug) {
+	if (ctl->verbose) {
 		printf(_
 		       ("Last drift adjustment done at %ld seconds after 1969\n"),
 		       (long)adjtime_p->last_adj_time);
@@ -268,12 +268,12 @@ static int synchronize_to_clock_tick(const struct hwclock_control *ctl)
 {
 	int rc;
 
-	if (ctl->debug)
+	if (ctl->verbose)
 		printf(_("Waiting for clock tick...\n"));
 
 	rc = ur->synchronize_to_clock_tick(ctl);
 
-	if (ctl->debug) {
+	if (ctl->verbose) {
 		if (rc)
 			printf(_("...synchronization failed\n"));
 		else
@@ -317,14 +317,14 @@ mktime_tz(const struct hwclock_control *ctl, struct tm tm,
 		 * mktime() returns -1).
 		 */
 		valid = 0;
-		if (ctl->debug)
+		if (ctl->verbose)
 			printf(_("Invalid values in hardware clock: "
 				 "%4d/%.2d/%.2d %.2d:%.2d:%.2d\n"),
 			       tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
 			       tm.tm_hour, tm.tm_min, tm.tm_sec);
 	} else {
 		valid = 1;
-		if (ctl->debug)
+		if (ctl->verbose)
 			printf(_
 			       ("Hw clock time : %4d/%.2d/%.2d %.2d:%.2d:%.2d = "
 				"%ld seconds since 1969\n"), tm.tm_year + 1900,
@@ -351,7 +351,7 @@ read_hardware_clock(const struct hwclock_control *ctl,
 	if (err)
 		return err;
 
-	if (ctl->debug)
+	if (ctl->verbose)
 		printf(_
 		       ("Time read from Hardware Clock: %4d/%.2d/%.2d %02d:%02d:%02d\n"),
 		       tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, tm.tm_hour,
@@ -379,7 +379,7 @@ set_hardware_clock(const struct hwclock_control *ctl, const time_t newtime)
 	else
 		localtime_r(&newtime, &new_broken_time);
 
-	if (ctl->debug)
+	if (ctl->verbose)
 		printf(_("Setting Hardware Clock to %.2d:%.2d:%.2d "
 			 "= %ld seconds since 1969\n"),
 		       new_broken_time.tm_hour, new_broken_time.tm_min,
@@ -474,7 +474,7 @@ set_hardware_clock_exact(const struct hwclock_control *ctl,
 		double ticksize;
 
 		/* FOR TESTING ONLY: inject random delays of up to 1000ms */
-		if (ctl->debug >= 10) {
+		if (ctl->verbose >= 10) {
 			int usec = random() % 1000000;
 			printf(_("sleeping ~%d usec\n"), usec);
 			xusleep(usec);
@@ -486,7 +486,7 @@ set_hardware_clock_exact(const struct hwclock_control *ctl,
 		prevsystime = nowsystime;
 
 		if (ticksize < 0) {
-			if (ctl->debug)
+			if (ctl->verbose)
 				printf(_("time jumped backward %.6f seconds "
 					 "to %ld.%06ld - retargeting\n"),
 				       ticksize, nowsystime.tv_sec,
@@ -494,7 +494,7 @@ set_hardware_clock_exact(const struct hwclock_control *ctl,
 			/* The retarget is handled at the end of the loop. */
 		} else if (deltavstarget < 0) {
 			/* deltavstarget < 0 if current time < target time */
-			if (ctl->debug >= 9)
+			if (ctl->verbose >= 9)
 				printf(_("%ld.%06ld < %ld.%06ld (%.6f)\n"),
 				       nowsystime.tv_sec,
 				       nowsystime.tv_usec,
@@ -510,7 +510,7 @@ set_hardware_clock_exact(const struct hwclock_control *ctl,
 			 * We missed our window.  Increase the tolerance and
 			 * aim for the next opportunity.
 			 */
-			if (ctl->debug)
+			if (ctl->verbose)
 				printf(_("missed it - %ld.%06ld is too far "
 					 "past %ld.%06ld (%.6f > %.6f)\n"),
 				       nowsystime.tv_sec,
@@ -538,7 +538,7 @@ set_hardware_clock_exact(const struct hwclock_control *ctl,
 		    + (int)(time_diff(nowsystime, refsystime)
 			    - RTC_SET_DELAY_SECS /* don't count this */
 			    + 0.5 /* for rounding */);
-	if (ctl->debug)
+	if (ctl->verbose)
 		printf(_("%ld.%06ld is close enough to %ld.%06ld (%.6f < %.6f)\n"
 			 "Set RTC to %ld (%ld + %d; refsystime = %ld.%06ld)\n"),
 		       nowsystime.tv_sec, nowsystime.tv_usec,
@@ -607,7 +607,7 @@ set_system_clock(const struct hwclock_control *ctl,
 	localtime_r(&newtime.tv_sec, &broken);
 	minuteswest = -get_gmtoff(&broken) / 60;
 
-	if (ctl->debug) {
+	if (ctl->verbose) {
 		if (ctl->hctosys && !ctl->universal)
 			printf(_("Calling settimeofday(NULL, %d) to set "
 				 "persistent_clock_is_local.\n"), minuteswest);
@@ -662,17 +662,17 @@ adjust_drift_factor(const struct hwclock_control *ctl,
 		    const struct timeval hclocktime)
 {
 	if (!ctl->update) {
-		if (ctl->debug)
+		if (ctl->verbose)
 			printf(_("Not adjusting drift factor because the "
 				 "--update-drift option was not used.\n"));
 	} else if (adjtime_p->last_calib_time == 0) {
-		if (ctl->debug)
+		if (ctl->verbose)
 			printf(_("Not adjusting drift factor because last "
 				 "calibration time is zero,\n"
 				 "so history is bad and calibration startover "
 				 "is necessary.\n"));
 	} else if ((hclocktime.tv_sec - adjtime_p->last_calib_time) < 4 * 60 * 60) {
-		if (ctl->debug)
+		if (ctl->verbose)
 			printf(_("Not adjusting drift factor because it has "
 				 "been less than four hours since the last "
 				 "calibration.\n"));
@@ -710,14 +710,14 @@ adjust_drift_factor(const struct hwclock_control *ctl,
 
 		drift_factor = adjtime_p->drift_factor + factor_adjust;
 		if (fabs(drift_factor) > MAX_DRIFT) {
-			if (ctl->debug)
+			if (ctl->verbose)
 				printf(_("Clock drift factor was calculated as "
 					 "%f seconds/day.\n"
 					 "It is far too much. Resetting to zero.\n"),
 				       drift_factor);
 			drift_factor = 0;
 		} else {
-			if (ctl->debug)
+			if (ctl->verbose)
 				printf(_("Clock drifted %f seconds in the past "
 					 "%f seconds\nin spite of a drift factor of "
 					 "%f seconds/day.\n"
@@ -761,7 +761,7 @@ calculate_adjustment(const struct hwclock_control *ctl,
 	tdrift_p->tv_sec = (time_t) floor(exact_adjustment);
 	tdrift_p->tv_usec = (exact_adjustment -
 				 (double)tdrift_p->tv_sec) * 1E6;
-	if (ctl->debug) {
+	if (ctl->verbose) {
 		printf(P_("Time since last adjustment is %ld second\n",
 			"Time since last adjustment is %ld seconds\n",
 		       (systime - last_time)),
@@ -790,7 +790,7 @@ static int save_adjtime(const struct hwclock_control *ctl,
 		  adjtime->last_calib_time,
 		  (adjtime->local_utc == LOCAL) ? "LOCAL" : "UTC");
 
-	if (ctl->debug){
+	if (ctl->verbose){
 		printf(_("New %s data:\n%s"),
 		       ctl->adj_file_name, content);
 	}
@@ -836,11 +836,11 @@ do_adjustment(const struct hwclock_control *ctl, struct adjtime *adjtime_p,
 	      const struct timeval read_time)
 {
 	if (adjtime_p->last_adj_time == 0) {
-		if (ctl->debug)
+		if (ctl->verbose)
 			printf(_("Not setting clock because last adjustment time is zero, "
 				 "so history is bad.\n"));
 	} else if (fabs(adjtime_p->drift_factor) > MAX_DRIFT) {
-		if (ctl->debug)
+		if (ctl->verbose)
 			printf(_("Not setting clock because drift factor %f is far too high.\n"),
 				adjtime_p->drift_factor);
 	} else {
@@ -864,16 +864,16 @@ static void determine_clock_access_method(const struct hwclock_control *ctl)
 		ur = probe_for_rtc_clock(ctl);
 #endif
 	if (ur) {
-		if (ctl->debug)
+		if (ctl->verbose)
 			puts(ur->interface_name);
 
 	} else {
-		if (ctl->debug)
+		if (ctl->verbose)
 			printf(_("No usable clock interface found.\n"));
 		warnx(_("Cannot access the Hardware Clock via "
 			"any known method."));
-		if (!ctl->debug)
-			warnx(_("Use the --debug option to see the "
+		if (!ctl->verbose)
+			warnx(_("Use the --verbose option to see the "
 				"details of our search for an access "
 				"method."));
 		hwclock_exit(ctl, EXIT_FAILURE);
@@ -922,7 +922,7 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 				     hclocktime.tv_sec, &tdrift);
 		hclocktime = time_inc(hclocktime, (double)
 				      -(tdrift.tv_sec + tdrift.tv_usec / 1E6));
-		if (ctl->debug) {
+		if (ctl->verbose) {
 			printf(_ ("Target date:   %ld\n"), set_time);
 			printf(_ ("Predicted RTC: %ld\n"), hclocktime.tv_sec);
 		}
@@ -1077,8 +1077,8 @@ usage(void)
 	       "     --noadjfile      do not use %1$s\n"), _PATH_ADJTIME);
 	printf(_(
 	       "     --adjfile <file> use an alternate file to %1$s\n"), _PATH_ADJTIME);
-	puts(_("     --test           dry run; implies --debug"));
-	puts(_(" -D, --debug          display more details"));
+	puts(_("     --test           dry run; implies --verbose"));
+	puts(_(" -v, --verbose        display more details"));
 	fputs(USAGE_SEPARATOR, stdout);
 	printf(USAGE_HELP_OPTIONS(22));
 	printf(USAGE_MAN_TAIL("hwclock(8)"));
@@ -1125,6 +1125,7 @@ int main(int argc, char **argv)
 		{ "version",      no_argument,       NULL, 'V'            },
 		{ "systohc",      no_argument,       NULL, 'w'            },
 		{ "debug",        no_argument,       NULL, 'D'            },
+		{ "verbose",      no_argument,       NULL, 'v'            },
 		{ "set",          no_argument,       NULL, OPT_SET        },
 #if defined(__linux__) && defined(__alpha__)
 		{ "getepoch",     no_argument,       NULL, OPT_GETEPOCH   },
@@ -1192,7 +1193,10 @@ int main(int argc, char **argv)
 
 		switch (c) {
 		case 'D':
-			ctl.debug++;
+			warnx(_("use --verbose, --debug has been deprecated."));
+			break;
+		case 'v':
+			ctl.verbose++;
 			break;
 		case 'a':
 			ctl.adjust = 1;
@@ -1245,7 +1249,7 @@ int main(int argc, char **argv)
 			break;
 		case OPT_TEST:
 			ctl.testing = 1;	/* --test */
-			ctl.debug++;
+			ctl.verbose++;
 			break;
 		case OPT_DATE:
 			ctl.date_opt = optarg;	/* --date */
@@ -1274,8 +1278,7 @@ int main(int argc, char **argv)
 			ctl.rtc_dev_name = optarg;	/* --rtc */
 			break;
 #endif
-		case 'v':			/* --version */
-		case 'V':
+		case 'V':			/* --version */
 			out_version();
 			return 0;
 		case 'h':			/* --help */
@@ -1324,7 +1327,7 @@ int main(int argc, char **argv)
 	}
 #endif
 
-	if (ctl.debug) {
+	if (ctl.verbose) {
 		out_version();
 		printf(_("System Time: %ld.%06ld\n"),
 		       startup_time.tv_sec, startup_time.tv_usec);
diff --git a/sys-utils/hwclock.h b/sys-utils/hwclock.h
index 215cf9302..570bfe439 100644
--- a/sys-utils/hwclock.h
+++ b/sys-utils/hwclock.h
@@ -18,7 +18,7 @@ struct hwclock_control {
 #ifdef __linux__
 	char *rtc_dev_name;
 #endif
-	unsigned int debug;
+	unsigned int verbose;
 	unsigned int
 		hwaudit_on:1,
 		adjust:1,
@@ -54,7 +54,6 @@ extern struct clock_ops *probe_for_cmos_clock(void);
 extern struct clock_ops *probe_for_rtc_clock(const struct hwclock_control *ctl);
 
 /* hwclock.c */
-extern int debug;
 extern double time_diff(struct timeval subtrahend, struct timeval subtractor);
 
 /* rtc.c */

[PATCH 2/2] hwclock: add --ul-debug implementing debug.h

[J William Piggott]

Undocumented at this time, because it is a skeleton
implementation.  More debugging points are to be added after
refactoring is complete, or ad hoc in the mean time.

When fully implemented, enough time may have passed that the
deprecated --debug could be used to replace --ul-debug.

Coauthored-by: Sami Kerola <kerolasa@iki.fi>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
Signed-off-by: J William Piggott <elseifthen@gmx.com>
---
 sys-utils/hwclock.c | 40 ++++++++++++++++++++++++----------------
 sys-utils/hwclock.h | 13 +++++++++++--
 2 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 0dc1d2369..90eab9f89 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -84,6 +84,8 @@
 static int hwaudit_fd = -1;
 #endif
 
+UL_DEBUG_DEFINE_MASK(hwclock);
+
 /* The struct that holds our hardware access routines */
 static struct clock_ops *ur;
 
@@ -120,6 +122,15 @@ struct adjtime {
 	 */
 };
 
+/* FOR TESTING ONLY: inject random delays of up to 1000ms */
+static void up_to_1000ms_sleep(void)
+{
+	int usec = random() % 1000000;
+
+	DBG(RANDOM_SLEEP, ul_debug("sleeping ~%d usec", usec));
+	xusleep(usec);
+}
+
 /*
  * time_t to timeval conversion.
  */
@@ -473,12 +484,7 @@ set_hardware_clock_exact(const struct hwclock_control *ctl,
 	while (1) {
 		double ticksize;
 
-		/* FOR TESTING ONLY: inject random delays of up to 1000ms */
-		if (ctl->verbose >= 10) {
-			int usec = random() % 1000000;
-			printf(_("sleeping ~%d usec\n"), usec);
-			xusleep(usec);
-		}
+		ON_DBG(RANDOM_SLEEP, up_to_1000ms_sleep());
 
 		gettimeofday(&nowsystime, NULL);
 		deltavstarget = time_diff(nowsystime, targetsystime);
@@ -494,13 +500,11 @@ set_hardware_clock_exact(const struct hwclock_control *ctl,
 			/* The retarget is handled at the end of the loop. */
 		} else if (deltavstarget < 0) {
 			/* deltavstarget < 0 if current time < target time */
-			if (ctl->verbose >= 9)
-				printf(_("%ld.%06ld < %ld.%06ld (%.6f)\n"),
-				       nowsystime.tv_sec,
-				       nowsystime.tv_usec,
-				       targetsystime.tv_sec,
-				       targetsystime.tv_usec,
-				       deltavstarget);
+			DBG(DELTA_VS_TARGET,
+			    ul_debug("%ld.%06ld < %ld.%06ld (%.6f)",
+				     nowsystime.tv_sec, nowsystime.tv_usec,
+				     targetsystime.tv_sec,
+				     targetsystime.tv_usec, deltavstarget));
 			continue;  /* not there yet - keep spinning */
 		} else if (deltavstarget <= target_time_tolerance_secs) {
 			/* Close enough to the target time; done waiting. */
@@ -1125,6 +1129,7 @@ int main(int argc, char **argv)
 		{ "version",      no_argument,       NULL, 'V'            },
 		{ "systohc",      no_argument,       NULL, 'w'            },
 		{ "debug",        no_argument,       NULL, 'D'            },
+		{ "ul-debug",     required_argument, NULL, 'd'            },
 		{ "verbose",      no_argument,       NULL, 'v'            },
 		{ "set",          no_argument,       NULL, OPT_SET        },
 #if defined(__linux__) && defined(__alpha__)
@@ -1187,7 +1192,7 @@ int main(int argc, char **argv)
 	atexit(close_stdout);
 
 	while ((c = getopt_long(argc, argv,
-				"hvVDalrsuwf:", longopts, NULL)) != -1) {
+				"hvVDd:alrsuwf:", longopts, NULL)) != -1) {
 
 		err_exclusive_options(c, longopts, excl, excl_st);
 
@@ -1196,7 +1201,10 @@ int main(int argc, char **argv)
 			warnx(_("use --verbose, --debug has been deprecated."));
 			break;
 		case 'v':
-			ctl.verbose++;
+			ctl.verbose = 1;
+			break;
+		case 'd':
+			hwclock_debug_mask = atoi(optarg);
 			break;
 		case 'a':
 			ctl.adjust = 1;
@@ -1249,7 +1257,7 @@ int main(int argc, char **argv)
 			break;
 		case OPT_TEST:
 			ctl.testing = 1;	/* --test */
-			ctl.verbose++;
+			ctl.verbose = 1;
 			break;
 		case OPT_DATE:
 			ctl.date_opt = optarg;	/* --date */
diff --git a/sys-utils/hwclock.h b/sys-utils/hwclock.h
index 570bfe439..f60fce7bd 100644
--- a/sys-utils/hwclock.h
+++ b/sys-utils/hwclock.h
@@ -8,6 +8,15 @@
 #include <time.h>
 
 #include "c.h"
+#include "debug.h"
+
+#define HWCLOCK_DEBUG_RANDOM_SLEEP	(1 << 0)
+#define HWCLOCK_DEBUG_DELTA_VS_TARGET	(1 << 1)
+#define HWCLOCK_DEBUG_ALL		0xFFFF
+
+UL_DEBUG_DECLARE_MASK(hwclock);
+#define DBG(m, x)	__UL_DBG(hwclock, HWCLOCK_DEBUG_, m, x)
+#define ON_DBG(m, x)	__UL_DBG_CALL(hwclock, HWCLOCK_DEBUG_, m, x)
 
 struct hwclock_control {
 	char *date_opt;
@@ -18,7 +27,6 @@ struct hwclock_control {
 #ifdef __linux__
 	char *rtc_dev_name;
 #endif
-	unsigned int verbose;
 	unsigned int
 		hwaudit_on:1,
 		adjust:1,
@@ -39,7 +47,8 @@ struct hwclock_control {
 		get:1,
 		set:1,
 		update:1,
-		universal:1;	/* will store hw_clock_is_utc() return value */
+		universal:1,	/* will store hw_clock_is_utc() return value */
+		verbose:1;
 };
 
 struct clock_ops {

Re: [PATCH 2/2] hwclock: add --ul-debug implementing debug.h

[Karel Zak]

On Sun, Dec 24, 2017 at 03:38:58PM -0500, J William Piggott wrote:
> 
> Undocumented at this time, because it is a skeleton
> implementation.  More debugging points are to be added after
> refactoring is complete, or ad hoc in the mean time.
> 
> When fully implemented, enough time may have passed that the
> deprecated --debug could be used to replace --ul-debug.

We use everywhere env.variables (e.g. HWCLOCK_DEBUG=). The macro
__UL_INIT_DEBUG() already provides all necessary functionality for
this purpose. 

It also allows to use human readable names for the masks or hex
numbers. See for example  sys-utils/lsns.c or libsmartcols/src/init.c
for more complex usage. The human readable masks are optional and now
used by shared libs only, see for example "LIBSMARTCOLS_DEBUG=help
lsns".

I'd like to keep things consistent and with no exceptions...

IMHO "hwclock --verbose" is good idea for ordinary users and
"HWCLOCK_DEBUG=all hwclock" for developers.

    Karel

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

Re: [PATCH 2/2] hwclock: add --ul-debug implementing debug.h

[J William Piggott]

On 12/29/2017 06:38 AM, Karel Zak wrote:
> On Sun, Dec 24, 2017 at 03:38:58PM -0500, J William Piggott wrote:
>>
>> Undocumented at this time, because it is a skeleton
>> implementation.  More debugging points are to be added after
>> refactoring is complete, or ad hoc in the mean time.
>>
>> When fully implemented, enough time may have passed that the
>> deprecated --debug could be used to replace --ul-debug.
> 
> We use everywhere env.variables (e.g. HWCLOCK_DEBUG=). The macro
> __UL_INIT_DEBUG() already provides all necessary functionality for
> this purpose. 
> 
> It also allows to use human readable names for the masks or hex
> numbers. See for example  sys-utils/lsns.c or libsmartcols/src/init.c
> for more complex usage. The human readable masks are optional and now
> used by shared libs only, see for example "LIBSMARTCOLS_DEBUG=help
> lsns".
> 
> I'd like to keep things consistent and with no exceptions...
> 
> IMHO "hwclock --verbose" is good idea for ordinary users and
> "HWCLOCK_DEBUG=all hwclock" for developers.
> 
>     Karel
>

Karel,

Sami sent me a patch doing exactly that. So I made time to digest debug.h; then
I replied that I had reversed my position; I don't like debug.h for hwclock.

I slept on it, and thought perhaps using an option instead of an env might be
an acceptable compromise. So I figured out how to make debug.h use an option.

Would it not be 'consistent' to say that commands use a debug option, and libs
use a debug env?

Consistent in this case isn't about the code, but the UI; correct? So doesn't
consistent really translate to convenient for developers. That does not seem
like a strong argument to me. A proportionate counter point could be that it is
more convenient to enter 'hwclock -d all' then 'HWCLOCK_DEBUG=all hwclock'.

I'm more interested in asking: how does using an env over an option benefit the
code? I think it does not. Commands already have the code to parse options;
adding more code to parse an env which serves no benefit to the application,
IMO, is code bloat (and possibly a security risk).

What I dislike the most is that debugging is very seldom used, yet debugging is
initialized unconditionally whether it is wanted or not. This is completely
unnecessary; an option does the job just as well without the added overhead.
For time sensitive commands like hwclock this is just one more place for
preemption to slip in. The one thing left that I really want to fix on hwclock
it its precision.  At least some of that problem is preemption. Using an option
over an env eliminates that.

My approach to debug.h adds a single line to hwclock.c. It eliminates the
unconditional debugging initialization and the other unnecessary code.

Regarding debug.h in general, is this an elegant solution?
  disk-utils/cfdisk.c:2691
	fdisk_init_debug(0);
	scols_init_debug(0);
	cfdisk_init_debug();

CFDISK_DEBUG=all LIBFDISK_DEBUG=all LIBBLKID_DEBUG=all \
LIBSMARTCOLS_DEBUG=all LIBSMARTCOLS_DEBUG_PADDING=on cfdisk

Is anyone else (other than e2fsprogs) using this approach?

I also wonder if the way debug.h is using the env argument is a potential security
whole? I didn't note any input sanitizing being done?

In the end I just do not like the debug.h solution. It's a solution in search
of a problem; a problem that commands do not have. I understand the concept for
libraries, but I'm not convinced that it is the best solution for them either.

Anyway, Sami has a patch queued that uses the env, if that is what you want.
Please remove my name from his commit messages, as I object to using debug.h in
hwclock.

Re: [PATCH 2/2] hwclock: add --ul-debug implementing debug.h

[Sami Kerola]

On 6 January 2018 at 16:11, J William Piggott <elseifthen@gmx.com> wrote:
>
>
> On 12/29/2017 06:38 AM, Karel Zak wrote:
>> On Sun, Dec 24, 2017 at 03:38:58PM -0500, J William Piggott wrote:
>>>
>>> Undocumented at this time, because it is a skeleton
>>> implementation.  More debugging points are to be added after
>>> refactoring is complete, or ad hoc in the mean time.
>>>
>>> When fully implemented, enough time may have passed that the
>>> deprecated --debug could be used to replace --ul-debug.
>>
>> We use everywhere env.variables (e.g. HWCLOCK_DEBUG=). The macro
>> __UL_INIT_DEBUG() already provides all necessary functionality for
>> this purpose.
>>
>> It also allows to use human readable names for the masks or hex
>> numbers. See for example  sys-utils/lsns.c or libsmartcols/src/init.c
>> for more complex usage. The human readable masks are optional and now
>> used by shared libs only, see for example "LIBSMARTCOLS_DEBUG=help
>> lsns".
>>
>> I'd like to keep things consistent and with no exceptions...
>>
>> IMHO "hwclock --verbose" is good idea for ordinary users and
>> "HWCLOCK_DEBUG=all hwclock" for developers.
>>
>>     Karel
>>
>
> Karel,
>
> Sami sent me a patch doing exactly that. So I made time to digest debug.h; then
> I replied that I had reversed my position; I don't like debug.h for hwclock.
>
> I slept on it, and thought perhaps using an option instead of an env might be
> an acceptable compromise. So I figured out how to make debug.h use an option.
>
> Would it not be 'consistent' to say that commands use a debug option, and libs
> use a debug env?
>
> Consistent in this case isn't about the code, but the UI; correct? So doesn't
> consistent really translate to convenient for developers. That does not seem
> like a strong argument to me. A proportionate counter point could be that it is
> more convenient to enter 'hwclock -d all' then 'HWCLOCK_DEBUG=all hwclock'.
>
> I'm more interested in asking: how does using an env over an option benefit the
> code? I think it does not. Commands already have the code to parse options;
> adding more code to parse an env which serves no benefit to the application,
> IMO, is code bloat (and possibly a security risk).
>
> What I dislike the most is that debugging is very seldom used, yet debugging is
> initialized unconditionally whether it is wanted or not. This is completely
> unnecessary; an option does the job just as well without the added overhead.
> For time sensitive commands like hwclock this is just one more place for
> preemption to slip in. The one thing left that I really want to fix on hwclock
> it its precision.  At least some of that problem is preemption. Using an option
> over an env eliminates that.
>
> My approach to debug.h adds a single line to hwclock.c. It eliminates the
> unconditional debugging initialization and the other unnecessary code.
>
> Regarding debug.h in general, is this an elegant solution?
>   disk-utils/cfdisk.c:2691
>         fdisk_init_debug(0);
>         scols_init_debug(0);
>         cfdisk_init_debug();
>
> CFDISK_DEBUG=all LIBFDISK_DEBUG=all LIBBLKID_DEBUG=all \
> LIBSMARTCOLS_DEBUG=all LIBSMARTCOLS_DEBUG_PADDING=on cfdisk
>
> Is anyone else (other than e2fsprogs) using this approach?
>
> I also wonder if the way debug.h is using the env argument is a potential security
> whole? I didn't note any input sanitizing being done?
>
> In the end I just do not like the debug.h solution. It's a solution in search
> of a problem; a problem that commands do not have. I understand the concept for
> libraries, but I'm not convinced that it is the best solution for them either.
>
> Anyway, Sami has a patch queued that uses the env, if that is what you want.
> Please remove my name from his commit messages, as I object to using debug.h in
> hwclock.

Demonstrative debug.h work that is referred in previous message can ben found
from here:

git://github.com/kerolasa/lelux-utiliteetit.git hwclock-debug

The last commit 'hwclock: add cmos debug support' should not be merged. It is
does too much messaging in sections where delays are not welcome. That commit
was supposed to show how to get debug.h to work across source files. IMHO this
branch is not quite good enough to be merged. What is needed is messaging at
lines that make sense and is useful, and hwclock-rtc.c messaging.

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

Re: [PATCH 2/2] hwclock: add --ul-debug implementing debug.h

[Karel Zak]

On Sat, Jan 06, 2018 at 11:11:31AM -0500, J William Piggott wrote:
> Sami sent me a patch doing exactly that. So I made time to digest debug.h; then
> I replied that I had reversed my position; I don't like debug.h for hwclock.

The debugging is mostly about messages in the code and I think debug.h
provides good infrastructure for this purpose. I don't think that the
trigger (env.var or --any-option) should be the reason to ignore
debug.h. Right? :-)

> I slept on it, and thought perhaps using an option instead of an env might be
> an acceptable compromise. So I figured out how to make debug.h use an option.
> 
> Would it not be 'consistent' to say that commands use a debug option, and libs
> use a debug env?

For libs it's definitely better to use env. vars, for command I have
no so strong opinion about it; and yes, I can probably accept --debug, 
but not sure how you want to do that for hwclock, --ul-debug is ugly.

> Consistent in this case isn't about the code, but the UI; correct? So doesn't
> consistent really translate to convenient for developers. That does not seem
> like a strong argument to me. A proportionate counter point could be that it is
> more convenient to enter 'hwclock -d all' then 'HWCLOCK_DEBUG=all hwclock'.

For me env.variable is something more hidden, less visible for end users :-)

> I'm more interested in asking: how does using an env over an option benefit the
> code? I think it does not. Commands already have the code to parse options;
> adding more code to parse an env which serves no benefit to the application,
> IMO, is code bloat (and possibly a security risk).

This is all in debug.h macros, you don't have to add any extra code to
your command. For --debug you have to add getopt stuff.

> What I dislike the most is that debugging is very seldom used, yet debugging is
> initialized unconditionally whether it is wanted or not. This is completely
> unnecessary; an option does the job just as well without the added overhead.

It's one getenv() call, nothing else. See libc for how many env.vars
it's sensitive.

> For time sensitive commands like hwclock this is just one more place for
> preemption to slip in. The one thing left that I really want to fix on hwclock
> it its precision.  At least some of that problem is preemption. Using an option
> over an env eliminates that.
> 
> My approach to debug.h adds a single line to hwclock.c. It eliminates the
> unconditional debugging initialization and the other unnecessary code.

If we want to support an option to trigger debug output than it would
be nice to extend debug.h and add __UL_INIT_DEBUG_FROM_STRING() and
keep it compatible (same) as the original __UL_INIT_DEBUG().
It would be better than atoi().

I'll accept a patch with this extension.

> Regarding debug.h in general, is this an elegant solution?
>   disk-utils/cfdisk.c:2691
> 	fdisk_init_debug(0);
> 	scols_init_debug(0);
> 	cfdisk_init_debug();
> 
> CFDISK_DEBUG=all LIBFDISK_DEBUG=all LIBBLKID_DEBUG=all \
> LIBSMARTCOLS_DEBUG=all LIBSMARTCOLS_DEBUG_PADDING=on cfdisk

I cannot imagine situation when you want to debug all this stuff
together. 

> Is anyone else (other than e2fsprogs) using this approach?

libs has tons of env.variables

> I also wonder if the way debug.h is using the env argument is a potential security
> whole? I didn't note any input sanitizing being done?

The debug messages should not contain any security sensitive stuff
(like passwords or so), enabled debugging should not enable any
functionality, etc. It's just fprintf(stderr, "Hello world I'm doing
this now...")  and everything else is bug.

    Karel

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

Re: [PATCH 1/2] hwclock: rename --debug option to --verbose

[Karel Zak]

On Sun, Dec 24, 2017 at 03:37:36PM -0500, J William Piggott wrote:
> 
> Warn on --debug; do not fallthrough because
> the message is lost in the verbose output.

Merged. 

It's change in options :-) but in this case the original option prints
a warning (and nowhere is guaranty of number and format of the debug
messages) and another functionality is not affected. So, I'm fine with
this change.

    Karel

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

Re: [PATCH 2/2] hwclock: add --ul-debug implementing debug.h

[Karel Zak]

On Sun, Dec 24, 2017 at 03:38:58PM -0500, J William Piggott wrote:
> 
> Undocumented at this time, because it is a skeleton
> implementation.  More debugging points are to be added after
> refactoring is complete, or ad hoc in the mean time.
> 
> When fully implemented, enough time may have passed that the
> deprecated --debug could be used to replace --ul-debug.

I have updated your patch to use a new __UL_INIT_DEBUG_FROM_STRING(),
so the mask is initialized from --ul-debug in way compatible with 
rest of the util-linux, but without getenv().

        ./hwclock --ul-debug all
        25183: hwclock:     INIT: hwclock debug mask: 0xffff
        25183: hwclock:     INIT: hwclock version: util-linux 2.31.188-b0fee
        2018-01-17 14:22:24.311689+01:00

If you agree I'll apply the patch to the master branch. IMHO it's good
compromise.

    Karel


>From 3eac099c14339e0e934353d2f3f0917fcabb787c Mon Sep 17 00:00:00 2001
From: J William Piggott <elseifthen@gmx.com>
Date: Sun, 24 Dec 2017 15:38:58 -0500
Subject: [PATCH] hwclock: add --ul-debug implementing debug.h

Undocumented at this time, because it is a skeleton
implementation.  More debugging points are to be added after
refactoring is complete, or ad hoc in the mean time.

When fully implemented, enough time may have passed that the
deprecated --debug could be used to replace --ul-debug.

[kzak@redhat.com: - use __UL_INIT_DEBUG_FROM_STRING() to initialize the mask
                  - add hwclock_init_debug()]

Coauthored-by: Sami Kerola <kerolasa@iki.fi>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
Signed-off-by: J William Piggott <elseifthen@gmx.com>
Signed-off-by: Karel Zak <kzak@redhat.com>
---
 sys-utils/hwclock.c | 49 +++++++++++++++++++++++++++++++++----------------
 sys-utils/hwclock.h | 14 ++++++++++++--
 2 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 0dc1d2369..b83e71004 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -84,6 +84,9 @@
 static int hwaudit_fd = -1;
 #endif
 
+UL_DEBUG_DEFINE_MASK(hwclock);
+UL_DEBUG_DEFINE_MASKNAMES(hwclock) = UL_DEBUG_EMPTY_MASKNAMES;
+
 /* The struct that holds our hardware access routines */
 static struct clock_ops *ur;
 
@@ -120,6 +123,23 @@ struct adjtime {
 	 */
 };
 
+static void hwclock_init_debug(const char *str)
+{
+	__UL_INIT_DEBUG_FROM_STRING(hwclock, HWCLOCK_DEBUG_, 0, str);
+
+	DBG(INIT, ul_debug("hwclock debug mask: 0x%04x", hwclock_debug_mask));
+	DBG(INIT, ul_debug("hwclock version: %s", PACKAGE_STRING));
+}
+
+/* FOR TESTING ONLY: inject random delays of up to 1000ms */
+static void up_to_1000ms_sleep(void)
+{
+	int usec = random() % 1000000;
+
+	DBG(RANDOM_SLEEP, ul_debug("sleeping ~%d usec", usec));
+	xusleep(usec);
+}
+
 /*
  * time_t to timeval conversion.
  */
@@ -473,12 +493,7 @@ set_hardware_clock_exact(const struct hwclock_control *ctl,
 	while (1) {
 		double ticksize;
 
-		/* FOR TESTING ONLY: inject random delays of up to 1000ms */
-		if (ctl->verbose >= 10) {
-			int usec = random() % 1000000;
-			printf(_("sleeping ~%d usec\n"), usec);
-			xusleep(usec);
-		}
+		ON_DBG(RANDOM_SLEEP, up_to_1000ms_sleep());
 
 		gettimeofday(&nowsystime, NULL);
 		deltavstarget = time_diff(nowsystime, targetsystime);
@@ -494,13 +509,11 @@ set_hardware_clock_exact(const struct hwclock_control *ctl,
 			/* The retarget is handled at the end of the loop. */
 		} else if (deltavstarget < 0) {
 			/* deltavstarget < 0 if current time < target time */
-			if (ctl->verbose >= 9)
-				printf(_("%ld.%06ld < %ld.%06ld (%.6f)\n"),
-				       nowsystime.tv_sec,
-				       nowsystime.tv_usec,
-				       targetsystime.tv_sec,
-				       targetsystime.tv_usec,
-				       deltavstarget);
+			DBG(DELTA_VS_TARGET,
+			    ul_debug("%ld.%06ld < %ld.%06ld (%.6f)",
+				     nowsystime.tv_sec, nowsystime.tv_usec,
+				     targetsystime.tv_sec,
+				     targetsystime.tv_usec, deltavstarget));
 			continue;  /* not there yet - keep spinning */
 		} else if (deltavstarget <= target_time_tolerance_secs) {
 			/* Close enough to the target time; done waiting. */
@@ -1125,6 +1138,7 @@ int main(int argc, char **argv)
 		{ "version",      no_argument,       NULL, 'V'            },
 		{ "systohc",      no_argument,       NULL, 'w'            },
 		{ "debug",        no_argument,       NULL, 'D'            },
+		{ "ul-debug",     required_argument, NULL, 'd'            },
 		{ "verbose",      no_argument,       NULL, 'v'            },
 		{ "set",          no_argument,       NULL, OPT_SET        },
 #if defined(__linux__) && defined(__alpha__)
@@ -1187,7 +1201,7 @@ int main(int argc, char **argv)
 	atexit(close_stdout);
 
 	while ((c = getopt_long(argc, argv,
-				"hvVDalrsuwf:", longopts, NULL)) != -1) {
+				"hvVDd:alrsuwf:", longopts, NULL)) != -1) {
 
 		err_exclusive_options(c, longopts, excl, excl_st);
 
@@ -1196,7 +1210,10 @@ int main(int argc, char **argv)
 			warnx(_("use --verbose, --debug has been deprecated."));
 			break;
 		case 'v':
-			ctl.verbose++;
+			ctl.verbose = 1;
+			break;
+		case 'd':
+			hwclock_init_debug(optarg);
 			break;
 		case 'a':
 			ctl.adjust = 1;
@@ -1249,7 +1266,7 @@ int main(int argc, char **argv)
 			break;
 		case OPT_TEST:
 			ctl.testing = 1;	/* --test */
-			ctl.verbose++;
+			ctl.verbose = 1;
 			break;
 		case OPT_DATE:
 			ctl.date_opt = optarg;	/* --date */
diff --git a/sys-utils/hwclock.h b/sys-utils/hwclock.h
index 570bfe439..7bb6ec8bd 100644
--- a/sys-utils/hwclock.h
+++ b/sys-utils/hwclock.h
@@ -8,6 +8,16 @@
 #include <time.h>
 
 #include "c.h"
+#include "debug.h"
+
+#define HWCLOCK_DEBUG_INIT		(1 << 0)
+#define HWCLOCK_DEBUG_RANDOM_SLEEP	(1 << 1)
+#define HWCLOCK_DEBUG_DELTA_VS_TARGET	(1 << 2)
+#define HWCLOCK_DEBUG_ALL		0xFFFF
+
+UL_DEBUG_DECLARE_MASK(hwclock);
+#define DBG(m, x)	__UL_DBG(hwclock, HWCLOCK_DEBUG_, m, x)
+#define ON_DBG(m, x)	__UL_DBG_CALL(hwclock, HWCLOCK_DEBUG_, m, x)
 
 struct hwclock_control {
 	char *date_opt;
@@ -18,7 +28,6 @@ struct hwclock_control {
 #ifdef __linux__
 	char *rtc_dev_name;
 #endif
-	unsigned int verbose;
 	unsigned int
 		hwaudit_on:1,
 		adjust:1,
@@ -39,7 +48,8 @@ struct hwclock_control {
 		get:1,
 		set:1,
 		update:1,
-		universal:1;	/* will store hw_clock_is_utc() return value */
+		universal:1,	/* will store hw_clock_is_utc() return value */
+		verbose:1;
 };
 
 struct clock_ops {
-- 
2.13.6

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

Re: [PATCH 2/2] hwclock: add --ul-debug implementing debug.h

[J William Piggott]

On 01/08/2018 05:55 AM, Karel Zak wrote:
> On Sat, Jan 06, 2018 at 11:11:31AM -0500, J William Piggott wrote:
 
>> What I dislike the most is that debugging is very seldom used, yet debugging is
>> initialized unconditionally whether it is wanted or not. This is completely
>> unnecessary; an option does the job just as well without the added overhead.
> 
> It's one getenv() call, nothing else. See libc for how many env.vars
> it's sensitive.

For glibc it's mostly only to honor the POSIX environment as it is required to
do. It also uses secure_getenv(); I see that you've since added something
simular to debug.h. That is one type of security problem that I was talking
about; I think there could be more. The glibc manual says:

   The argv mechanism is typically used to pass command-line arguments specific
   to the particular program being invoked.  The environment, on the other
   hand, keeps track of information that is shared by many programs, changes
   infrequently, and that is less frequently used.

I wonder how a debug.h patch would be received on the libc-alpha list?

Anyway, I don't want to cause problems about this. You've made a compromise for
hwclock and I appreciate that. My comments are only food-for-thought, or as you
say brainstorming. I just think the general concept is not so good.

Re: [PATCH 2/2] hwclock: add --ul-debug implementing debug.h

[J William Piggott]

On 01/17/2018 08:29 AM, Karel Zak wrote:
> On Sun, Dec 24, 2017 at 03:38:58PM -0500, J William Piggott wrote:

> If you agree I'll apply the patch to the master branch. IMHO it's good
> compromise.

Yes, thank you Karel for this compromise.

> 
>     Karel
> 
> 
>>From 3eac099c14339e0e934353d2f3f0917fcabb787c Mon Sep 17 00:00:00 2001
> From: J William Piggott <elseifthen@gmx.com>
> Date: Sun, 24 Dec 2017 15:38:58 -0500
> Subject: [PATCH] hwclock: add --ul-debug implementing debug.h
> 
> Undocumented at this time, because it is a skeleton
> implementation.  More debugging points are to be added after
> refactoring is complete, or ad hoc in the mean time.
> 
> When fully implemented, enough time may have passed that the
> deprecated --debug could be used to replace --ul-debug.
> 
> [kzak@redhat.com: - use __UL_INIT_DEBUG_FROM_STRING() to initialize the mask
>                   - add hwclock_init_debug()]
> 
> Coauthored-by: Sami Kerola <kerolasa@iki.fi>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> Signed-off-by: J William Piggott <elseifthen@gmx.com>
> Signed-off-by: Karel Zak <kzak@redhat.com>
> ---
>  sys-utils/hwclock.c | 49 +++++++++++++++++++++++++++++++++----------------
>  sys-utils/hwclock.h | 14 ++++++++++++--
>  2 files changed, 45 insertions(+), 18 deletions(-)
> 
> diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
> index 0dc1d2369..b83e71004 100644
> --- a/sys-utils/hwclock.c
> +++ b/sys-utils/hwclock.c
> @@ -84,6 +84,9 @@
>  static int hwaudit_fd = -1;
>  #endif
>  
> +UL_DEBUG_DEFINE_MASK(hwclock);
> +UL_DEBUG_DEFINE_MASKNAMES(hwclock) = UL_DEBUG_EMPTY_MASKNAMES;
> +
>  /* The struct that holds our hardware access routines */
>  static struct clock_ops *ur;
>  
> @@ -120,6 +123,23 @@ struct adjtime {
>  	 */
>  };
>  
> +static void hwclock_init_debug(const char *str)
> +{
> +	__UL_INIT_DEBUG_FROM_STRING(hwclock, HWCLOCK_DEBUG_, 0, str);
> +
> +	DBG(INIT, ul_debug("hwclock debug mask: 0x%04x", hwclock_debug_mask));
> +	DBG(INIT, ul_debug("hwclock version: %s", PACKAGE_STRING));
> +}
> +
> +/* FOR TESTING ONLY: inject random delays of up to 1000ms */
> +static void up_to_1000ms_sleep(void)
> +{
> +	int usec = random() % 1000000;
> +
> +	DBG(RANDOM_SLEEP, ul_debug("sleeping ~%d usec", usec));
> +	xusleep(usec);
> +}
> +
>  /*
>   * time_t to timeval conversion.
>   */
> @@ -473,12 +493,7 @@ set_hardware_clock_exact(const struct hwclock_control *ctl,
>  	while (1) {
>  		double ticksize;
>  
> -		/* FOR TESTING ONLY: inject random delays of up to 1000ms */
> -		if (ctl->verbose >= 10) {
> -			int usec = random() % 1000000;
> -			printf(_("sleeping ~%d usec\n"), usec);
> -			xusleep(usec);
> -		}
> +		ON_DBG(RANDOM_SLEEP, up_to_1000ms_sleep());
>  
>  		gettimeofday(&nowsystime, NULL);
>  		deltavstarget = time_diff(nowsystime, targetsystime);
> @@ -494,13 +509,11 @@ set_hardware_clock_exact(const struct hwclock_control *ctl,
>  			/* The retarget is handled at the end of the loop. */
>  		} else if (deltavstarget < 0) {
>  			/* deltavstarget < 0 if current time < target time */
> -			if (ctl->verbose >= 9)
> -				printf(_("%ld.%06ld < %ld.%06ld (%.6f)\n"),
> -				       nowsystime.tv_sec,
> -				       nowsystime.tv_usec,
> -				       targetsystime.tv_sec,
> -				       targetsystime.tv_usec,
> -				       deltavstarget);
> +			DBG(DELTA_VS_TARGET,
> +			    ul_debug("%ld.%06ld < %ld.%06ld (%.6f)",
> +				     nowsystime.tv_sec, nowsystime.tv_usec,
> +				     targetsystime.tv_sec,
> +				     targetsystime.tv_usec, deltavstarget));
>  			continue;  /* not there yet - keep spinning */
>  		} else if (deltavstarget <= target_time_tolerance_secs) {
>  			/* Close enough to the target time; done waiting. */
> @@ -1125,6 +1138,7 @@ int main(int argc, char **argv)
>  		{ "version",      no_argument,       NULL, 'V'            },
>  		{ "systohc",      no_argument,       NULL, 'w'            },
>  		{ "debug",        no_argument,       NULL, 'D'            },
> +		{ "ul-debug",     required_argument, NULL, 'd'            },
>  		{ "verbose",      no_argument,       NULL, 'v'            },
>  		{ "set",          no_argument,       NULL, OPT_SET        },
>  #if defined(__linux__) && defined(__alpha__)
> @@ -1187,7 +1201,7 @@ int main(int argc, char **argv)
>  	atexit(close_stdout);
>  
>  	while ((c = getopt_long(argc, argv,
> -				"hvVDalrsuwf:", longopts, NULL)) != -1) {
> +				"hvVDd:alrsuwf:", longopts, NULL)) != -1) {
>  
>  		err_exclusive_options(c, longopts, excl, excl_st);
>  
> @@ -1196,7 +1210,10 @@ int main(int argc, char **argv)
>  			warnx(_("use --verbose, --debug has been deprecated."));
>  			break;
>  		case 'v':
> -			ctl.verbose++;
> +			ctl.verbose = 1;
> +			break;
> +		case 'd':
> +			hwclock_init_debug(optarg);
>  			break;
>  		case 'a':
>  			ctl.adjust = 1;
> @@ -1249,7 +1266,7 @@ int main(int argc, char **argv)
>  			break;
>  		case OPT_TEST:
>  			ctl.testing = 1;	/* --test */
> -			ctl.verbose++;
> +			ctl.verbose = 1;
>  			break;
>  		case OPT_DATE:
>  			ctl.date_opt = optarg;	/* --date */
> diff --git a/sys-utils/hwclock.h b/sys-utils/hwclock.h
> index 570bfe439..7bb6ec8bd 100644
> --- a/sys-utils/hwclock.h
> +++ b/sys-utils/hwclock.h
> @@ -8,6 +8,16 @@
>  #include <time.h>
>  
>  #include "c.h"
> +#include "debug.h"
> +
> +#define HWCLOCK_DEBUG_INIT		(1 << 0)
> +#define HWCLOCK_DEBUG_RANDOM_SLEEP	(1 << 1)
> +#define HWCLOCK_DEBUG_DELTA_VS_TARGET	(1 << 2)
> +#define HWCLOCK_DEBUG_ALL		0xFFFF
> +
> +UL_DEBUG_DECLARE_MASK(hwclock);
> +#define DBG(m, x)	__UL_DBG(hwclock, HWCLOCK_DEBUG_, m, x)
> +#define ON_DBG(m, x)	__UL_DBG_CALL(hwclock, HWCLOCK_DEBUG_, m, x)
>  
>  struct hwclock_control {
>  	char *date_opt;
> @@ -18,7 +28,6 @@ struct hwclock_control {
>  #ifdef __linux__
>  	char *rtc_dev_name;
>  #endif
> -	unsigned int verbose;
>  	unsigned int
>  		hwaudit_on:1,
>  		adjust:1,
> @@ -39,7 +48,8 @@ struct hwclock_control {
>  		get:1,
>  		set:1,
>  		update:1,
> -		universal:1;	/* will store hw_clock_is_utc() return value */
> +		universal:1,	/* will store hw_clock_is_utc() return value */
> +		verbose:1;
>  };
>  
>  struct clock_ops {
> 

Re: [PATCH 2/2] hwclock: add --ul-debug implementing debug.h

[Karel Zak]

On Sun, Jan 21, 2018 at 02:38:25PM -0500, J William Piggott wrote:
> I wonder how a debug.h patch would be received on the libc-alpha list?

Do you see any alpha specific issue there? 

> Anyway, I don't want to cause problems about this. You've made a compromise for
> hwclock and I appreciate that. My comments are only food-for-thought, or as you
> say brainstorming. I just think the general concept is not so good.

Well, it was designed for libs and later used (with no change) for
another code like lsblk or script... 

Anyway, merged to hwclock with --ul-debug.

    Karel


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