[PATCH 0/3] Pull Request

[PATCH 0/3] Pull Request

[J William Piggott]

The following changes since commit e406be01b450dd7ed5e694bc15a39719d6f86967:

  hwclock: for debugging print startup system time (2017-08-24 18:49:11 -0400)

are available in the git repository at:

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

for you to fetch changes up to 0396d0c2bc33a34d132b0d4f9f5f055668732989:

  hwclock: close hwaudit_fd unconditionally (2017-08-28 21:19:55 -0400)

----------------------------------------------------------------
J William Piggott (3):
      hwclock: remove sysexits.h
      hwclock: remove audit from control struct
      hwclock: close hwaudit_fd unconditionally

 sys-utils/hwclock-rtc.c |   5 +--
 sys-utils/hwclock.8.in  |   8 ++++
 sys-utils/hwclock.c     | 104 +++++++++++++++++++-----------------------------
 sys-utils/hwclock.h     |   3 +-
 4 files changed, 52 insertions(+), 68 deletions(-)

[PATCH 1/3] hwclock: remove sysexits.h

[J William Piggott]

sysexits.h was introduced in v2.11t prior to util-linux-ng, with the
HISTORY entry: * hwclock: minor polishing.

So there was no specific issue solved by adding it. Its use was never
documented so it should be safe to remove.

Also, fix return values being used for the exit status that were not
magic constants (portability issue).

Signed-off-by: J William Piggott <elseifthen@gmx.com>
---
 sys-utils/hwclock-rtc.c |  5 ++--
 sys-utils/hwclock.8.in  |  8 ++++++
 sys-utils/hwclock.c     | 68 +++++++++++++++++--------------------------------
 3 files changed, 33 insertions(+), 48 deletions(-)

diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
index c50011a..62e4329 100644
--- a/sys-utils/hwclock-rtc.c
+++ b/sys-utils/hwclock-rtc.c
@@ -6,7 +6,6 @@
 #include <fcntl.h>
 #include <stdio.h>
 #include <stdlib.h>
-#include <sysexits.h>
 #include <sys/ioctl.h>
 #include <sys/select.h>
 #include <sys/time.h>
@@ -149,7 +148,7 @@ static int open_rtc_or_exit(const struct hwclock_control *ctl)
 
 	if (rtc_fd < 0) {
 		warn(_("cannot open rtc device"));
-		hwclock_exit(ctl, EX_OSFILE);
+		hwclock_exit(ctl, EXIT_FAILURE);
 	}
 	return rtc_fd;
 }
@@ -356,7 +355,7 @@ static int set_hardware_clock_rtc(const struct hwclock_control *ctl,
 	if (rc == -1) {
 		warn(_("ioctl(%s) to %s to set the time failed"),
 			ioctlname, rtc_dev_name);
-		hwclock_exit(ctl, EX_IOERR);
+		hwclock_exit(ctl, EXIT_FAILURE);
 	}
 
 	if (ctl->debug)
diff --git a/sys-utils/hwclock.8.in b/sys-utils/hwclock.8.in
index 107e3f1..8bdc841 100644
--- a/sys-utils/hwclock.8.in
+++ b/sys-utils/hwclock.8.in
@@ -917,6 +917,14 @@ timescale. The zoneinfo database must be configured to use either posix
 or 'right', as described above, or by assigning a database path to the
 .SB TZDIR
 environment variable.
+.SH EXIT STATUS
+One of the following exit values will be returned:
+.TP
+.BR EXIT_SUCCESS " ('0' on POSIX systems)"
+Successful program execution.
+.TP
+.BR EXIT_FAILURE " ('1' on POSIX systems)"
+The operation failed or the command syntax was not valid.
 .SH ENVIRONMENT
 .TP
 .B TZ
diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 31cd027..277953f 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -64,15 +64,11 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <sysexits.h>
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <time.h>
 #include <unistd.h>
 
-#define OPTUTILS_EXIT_CODE EX_USAGE
-#define XALLOC_EXIT_CODE EX_OSERR
-
 #include "c.h"
 #include "closestream.h"
 #include "nls.h"
@@ -192,11 +188,8 @@ hw_clock_is_utc(const struct hwclock_control *ctl,
 /*
  * Read the adjustment parameters out of the /etc/adjtime file.
  *
- * Return them as the adjtime structure <*adjtime_p>. If there is no
- * /etc/adjtime file, return defaults. If values are missing from the file,
- * return defaults for them.
- *
- * return value 0 if all OK, !=0 otherwise.
+ * Return them as the adjtime structure <*adjtime_p>. Its defaults are
+ * initialized in main().
  */
 static int read_adjtime(const struct hwclock_control *ctl,
 			struct adjtime *adjtime_p)
@@ -207,12 +200,12 @@ static int read_adjtime(const struct hwclock_control *ctl,
 	char line3[81];		/* String: third line of adjtime file */
 
 	if (access(ctl->adj_file_name, R_OK) != 0)
-		return 0;
+		return EXIT_SUCCESS;
 
 	adjfile = fopen(ctl->adj_file_name, "r");	/* open file for reading */
 	if (adjfile == NULL) {
 		warn(_("cannot open %s"), ctl->adj_file_name);
-		return EX_OSFILE;
+		return EXIT_FAILURE;
 	}
 
 	if (!fgets(line1, sizeof(line1), adjfile))
@@ -255,7 +248,7 @@ static int read_adjtime(const struct hwclock_control *ctl,
 					       UTC) ? _("UTC") : _("unknown"));
 	}
 
-	return 0;
+	return EXIT_SUCCESS;
 }
 
 /*
@@ -654,10 +647,10 @@ set_system_clock(const struct hwclock_control *ctl,
 
 		if (rc) {
 			warn(_("settimeofday() failed"));
-			return  1;
+			return  EXIT_FAILURE;
 		}
 	}
-	return 0;
+	return EXIT_SUCCESS;
 }
 
 /*
@@ -900,17 +893,11 @@ static void determine_clock_access_method(const struct hwclock_control *ctl)
 			warnx(_("Use the --debug option to see the "
 				"details of our search for an access "
 				"method."));
-		hwclock_exit(ctl, EX_SOFTWARE);
+		hwclock_exit(ctl, EXIT_FAILURE);
 	}
 }
 
-/*
- * Do all the normal work of hwclock - read, set clock, etc.
- *
- * Issue output to stdout and error message to stderr where appropriate.
- *
- * Return rc == 0 if everything went OK, rc != 0 if not.
- */
+/* Do all the normal work of hwclock - read, set clock, etc. */
 static int
 manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 		 const struct timeval startup_time, struct adjtime *adjtime)
@@ -957,14 +944,14 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 			printf(_ ("Predicted RTC: %ld\n"), hclocktime.tv_sec);
 		}
 		display_time(hclocktime);
-		return 0;
+		return EXIT_SUCCESS;
 	}
 
 	if (ctl->systz)
 		return set_system_clock(ctl, startup_time);
 
 	if (ur->get_permissions())
-		return EX_NOPERM;
+		return EXIT_FAILURE;
 
 	/*
 	 * Read and drift correct RTC time; except for RTC set functions
@@ -980,13 +967,13 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 		 * operations are invalid without it.
 		 */
 		if (synchronize_to_clock_tick(ctl))
-			return EX_IOERR;
+			return EXIT_FAILURE;
 		read_hardware_clock(ctl, &hclock_valid, &hclocktime.tv_sec);
 		gettimeofday(&read_time, NULL);
 
 		if (!hclock_valid) {
 			warnx(_("RTC read returned an invalid value."));
-			return EX_IOERR;
+			return EXIT_FAILURE;
 		}
 		/*
 		 * Calculate and apply drift correction to the Hardware Clock
@@ -1035,7 +1022,7 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 	}
 	if (!ctl->noadjfile)
 		save_adjtime(ctl, adjtime);
-	return 0;
+	return EXIT_SUCCESS;
 }
 
 /**
@@ -1118,15 +1105,6 @@ usage(const struct hwclock_control *ctl)
 	hwclock_exit(ctl, EXIT_SUCCESS);
 }
 
-/*
- * Returns:
- *  EX_USAGE: bad invocation
- *  EX_NOPERM: no permission
- *  EX_OSFILE: cannot open /dev/rtc or /etc/adjtime
- *  EX_IOERR: ioctl error getting or setting the time
- *  0: OK (or not)
- *  1: failure
- */
 int main(int argc, char **argv)
 {
 	struct hwclock_control ctl = { .show = 1 }; /* default op is show */
@@ -1211,7 +1189,7 @@ int main(int argc, char **argv)
 		 * have audit compiled in.
 		 */
 		warnx(_("Unable to connect to audit system"));
-		return EX_NOPERM;
+		return EXIT_FAILURE;
 	}
 #endif
 	setlocale(LC_ALL, "");
@@ -1322,13 +1300,13 @@ int main(int argc, char **argv)
 		case 'h':			/* --help */
 			usage(&ctl);
 		default:
-			errtryhelp(EX_USAGE);
+			errtryhelp(EXIT_FAILURE);
 		}
 	}
 
 	if (argc -= optind) {
 		warnx(_("%d too many arguments given"), argc);
-		errtryhelp(EX_USAGE);
+		errtryhelp(EXIT_FAILURE);
 	}
 
 	if (!ctl.adj_file_name)
@@ -1336,32 +1314,32 @@ int main(int argc, char **argv)
 
 	if (ctl.update && !ctl.set && !ctl.systohc) {
 		warnx(_("--update-drift requires --set or --systohc"));
-		hwclock_exit(&ctl, EX_USAGE);
+		hwclock_exit(&ctl, EXIT_FAILURE);
 	}
 
 	if (ctl.noadjfile && !ctl.utc && !ctl.local_opt) {
 		warnx(_("With --noadjfile, you must specify "
 			"either --utc or --localtime"));
-		hwclock_exit(&ctl, EX_USAGE);
+		hwclock_exit(&ctl, EXIT_FAILURE);
 	}
 
 	if (ctl.set || ctl.predict) {
 		if (!ctl.date_opt) {
 		warnx(_("--date is required for --set or --predict"));
-		hwclock_exit(&ctl, EX_USAGE);
+		hwclock_exit(&ctl, EXIT_FAILURE);
 		}
 		if (parse_date(&when, ctl.date_opt, NULL))
 			set_time = when.tv_sec;
 		else {
 			warnx(_("invalid date '%s'"), ctl.date_opt);
-			hwclock_exit(&ctl, EX_USAGE);
+			hwclock_exit(&ctl, EXIT_FAILURE);
 		}
 	}
 
 #if defined(__linux__) && defined(__alpha__)
 	if (ctl.getepoch || ctl.setepoch) {
 		manipulate_epoch(&ctl);
-		hwclock_exit(&ctl, EX_OK);
+		hwclock_exit(&ctl, EXIT_SUCCESS);
 	}
 #endif
 
@@ -1397,7 +1375,7 @@ hwclock_exit(const struct hwclock_control *ctl
 	if (ctl->hwaudit_on && !ctl->testing) {
 		audit_log_user_message(hwaudit_fd, AUDIT_USYS_CONFIG,
 				       "op=change-system-time", NULL, NULL, NULL,
-				       status ? 0 : 1);
+				       status);
 		close(hwaudit_fd);
 	}
 #endif

[PATCH 2/3] hwclock: remove audit from control struct

[J William Piggott]

hwaudit_on is only used in main() and hwclock_exit() and
that is unlikely to ever change.

Remove it from the control struct because it:
 * cleans up an ugly directive in hwclock_exit
 * removes an argument from usage() and hwclock_exit()
 * makes usage() comply with util-linux standards

Signed-off-by: J William Piggott <elseifthen@gmx.com>
---
 sys-utils/hwclock-rtc.c |  4 ++--
 sys-utils/hwclock.c     | 46 +++++++++++++++++++++++-----------------------
 sys-utils/hwclock.h     |  3 +--
 3 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
index 62e4329..9d52e40 100644
--- a/sys-utils/hwclock-rtc.c
+++ b/sys-utils/hwclock-rtc.c
@@ -148,7 +148,7 @@ static int open_rtc_or_exit(const struct hwclock_control *ctl)
 
 	if (rtc_fd < 0) {
 		warn(_("cannot open rtc device"));
-		hwclock_exit(ctl, EXIT_FAILURE);
+		hwclock_exit(EXIT_FAILURE);
 	}
 	return rtc_fd;
 }
@@ -355,7 +355,7 @@ static int set_hardware_clock_rtc(const struct hwclock_control *ctl,
 	if (rc == -1) {
 		warn(_("ioctl(%s) to %s to set the time failed"),
 			ioctlname, rtc_dev_name);
-		hwclock_exit(ctl, EXIT_FAILURE);
+		hwclock_exit(EXIT_FAILURE);
 	}
 
 	if (ctl->debug)
diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 277953f..6f868df 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -83,6 +83,7 @@
 #include <libaudit.h>
 static int hwaudit_fd = -1;
 #endif
+static int hwaudit_on;
 
 /* The struct that holds our hardware access routines */
 static struct clock_ops *ur;
@@ -893,7 +894,7 @@ static void determine_clock_access_method(const struct hwclock_control *ctl)
 			warnx(_("Use the --debug option to see the "
 				"details of our search for an access "
 				"method."));
-		hwclock_exit(ctl, EXIT_FAILURE);
+		hwclock_exit(EXIT_FAILURE);
 	}
 }
 
@@ -1058,7 +1059,7 @@ static void out_version(void)
 }
 
 static void __attribute__((__noreturn__))
-usage(const struct hwclock_control *ctl)
+usage(void)
 {
 	fputs(USAGE_HEADER, stdout);
 	printf(_(" %s [function] [option...]\n"), program_invocation_short_name);
@@ -1102,7 +1103,7 @@ usage(const struct hwclock_control *ctl)
 	fputs(USAGE_SEPARATOR, stdout);
 	printf(USAGE_HELP_OPTIONS(22));
 	printf(USAGE_MAN_TAIL("hwclock(8)"));
-	hwclock_exit(ctl, EXIT_SUCCESS);
+	hwclock_exit(EXIT_SUCCESS);
 }
 
 int main(int argc, char **argv)
@@ -1217,7 +1218,7 @@ int main(int argc, char **argv)
 		case 'a':
 			ctl.adjust = 1;
 			ctl.show = 0;
-			ctl.hwaudit_on = 1;
+			hwaudit_on = 1;
 			break;
 		case 'l':
 			ctl.local_opt = 1;	/* --localtime */
@@ -1228,7 +1229,7 @@ int main(int argc, char **argv)
 		case 's':
 			ctl.hctosys = 1;
 			ctl.show = 0;
-			ctl.hwaudit_on = 1;
+			hwaudit_on = 1;
 			break;
 		case 'u':
 			ctl.utc = 1;
@@ -1236,12 +1237,12 @@ int main(int argc, char **argv)
 		case 'w':
 			ctl.systohc = 1;
 			ctl.show = 0;
-			ctl.hwaudit_on = 1;
+			hwaudit_on = 1;
 			break;
 		case OPT_SET:
 			ctl.set = 1;
 			ctl.show = 0;
-			ctl.hwaudit_on = 1;
+			hwaudit_on = 1;
 			break;
 #if defined(__linux__) && defined(__alpha__)
 		case OPT_GETEPOCH:
@@ -1251,7 +1252,7 @@ int main(int argc, char **argv)
 		case OPT_SETEPOCH:
 			ctl.setepoch = 1;
 			ctl.show = 0;
-			ctl.hwaudit_on = 1;
+			hwaudit_on = 1;
 			break;
 		case OPT_EPOCH:
 			ctl.epoch_option = optarg;	/* --epoch */
@@ -1275,7 +1276,7 @@ int main(int argc, char **argv)
 		case OPT_SYSTZ:
 			ctl.systz = 1;		/* --systz */
 			ctl.show = 0;
-			ctl.hwaudit_on = 1;
+			hwaudit_on = 1;
 			break;
 		case OPT_PREDICT:
 			ctl.predict = 1;	/* --predict */
@@ -1298,12 +1299,15 @@ int main(int argc, char **argv)
 			out_version();
 			return 0;
 		case 'h':			/* --help */
-			usage(&ctl);
+			usage();
 		default:
 			errtryhelp(EXIT_FAILURE);
 		}
 	}
 
+	if (ctl.testing)
+			hwaudit_on = 0;
+
 	if (argc -= optind) {
 		warnx(_("%d too many arguments given"), argc);
 		errtryhelp(EXIT_FAILURE);
@@ -1314,32 +1318,32 @@ int main(int argc, char **argv)
 
 	if (ctl.update && !ctl.set && !ctl.systohc) {
 		warnx(_("--update-drift requires --set or --systohc"));
-		hwclock_exit(&ctl, EXIT_FAILURE);
+		hwclock_exit(EXIT_FAILURE);
 	}
 
 	if (ctl.noadjfile && !ctl.utc && !ctl.local_opt) {
 		warnx(_("With --noadjfile, you must specify "
 			"either --utc or --localtime"));
-		hwclock_exit(&ctl, EXIT_FAILURE);
+		hwclock_exit(EXIT_FAILURE);
 	}
 
 	if (ctl.set || ctl.predict) {
 		if (!ctl.date_opt) {
 		warnx(_("--date is required for --set or --predict"));
-		hwclock_exit(&ctl, EXIT_FAILURE);
+		hwclock_exit(EXIT_FAILURE);
 		}
 		if (parse_date(&when, ctl.date_opt, NULL))
 			set_time = when.tv_sec;
 		else {
 			warnx(_("invalid date '%s'"), ctl.date_opt);
-			hwclock_exit(&ctl, EXIT_FAILURE);
+			hwclock_exit(EXIT_FAILURE);
 		}
 	}
 
 #if defined(__linux__) && defined(__alpha__)
 	if (ctl.getepoch || ctl.setepoch) {
 		manipulate_epoch(&ctl);
-		hwclock_exit(&ctl, EXIT_SUCCESS);
+		hwclock_exit(EXIT_SUCCESS);
 	}
 #endif
 
@@ -1354,25 +1358,21 @@ int main(int argc, char **argv)
 
 	if (!ctl.noadjfile && !(ctl.systz && (ctl.utc || ctl.local_opt))) {
 		if ((rc = read_adjtime(&ctl, &adjtime)) != 0)
-			hwclock_exit(&ctl, rc);
+			hwclock_exit(rc);
 	} else
 		/* Avoid writing adjtime file if we don't have to. */
 		adjtime.dirty = FALSE;
 	ctl.universal = hw_clock_is_utc(&ctl, adjtime);
 	rc = manipulate_clock(&ctl, set_time, startup_time, &adjtime);
-	hwclock_exit(&ctl, rc);
+	hwclock_exit(rc);
 	return rc;		/* Not reached */
 }
 
 void
-hwclock_exit(const struct hwclock_control *ctl
-#ifndef HAVE_LIBAUDIT
-	     __attribute__((__unused__))
-#endif
-	     , int status)
+hwclock_exit(int status)
 {
 #ifdef HAVE_LIBAUDIT
-	if (ctl->hwaudit_on && !ctl->testing) {
+	if (hwaudit_on) {
 		audit_log_user_message(hwaudit_fd, AUDIT_USYS_CONFIG,
 				       "op=change-system-time", NULL, NULL, NULL,
 				       status);
diff --git a/sys-utils/hwclock.h b/sys-utils/hwclock.h
index 6943d8d..013a7a3 100644
--- a/sys-utils/hwclock.h
+++ b/sys-utils/hwclock.h
@@ -20,7 +20,6 @@ struct hwclock_control {
 #endif
 	unsigned int debug;
 	unsigned int
-		hwaudit_on:1,
 		adjust:1,
 		show:1,
 		hctosys:1,
@@ -66,6 +65,6 @@ extern int set_epoch_rtc(const struct hwclock_control *ctl);
 #endif
 
 extern void __attribute__((__noreturn__))
-hwclock_exit(const struct hwclock_control *ctl, int status);
+hwclock_exit(int status);
 
 #endif				/* HWCLOCK_CLOCK_H */

Re: [PATCH 2/3] hwclock: remove audit from control struct

[Karel Zak]

On Tue, Aug 29, 2017 at 12:34:07PM -0400, J William Piggott wrote:
> 
> hwaudit_on is only used in main() and hwclock_exit() and
> that is unlikely to ever change.

 Hmm... the goal is minimize global variables to keep code readable.
 It's strange to have a control struct and use global variable in the
 same time.

> Remove it from the control struct because it:
>  * cleans up an ugly directive in hwclock_exit
>  * removes an argument from usage() and hwclock_exit()
>  * makes usage() comply with util-linux standards

 I think usage() can call exit(EXIT_SUCCESS) and do not need play any
 games with audit at all. 
 
 On another places we have "ctl" so it's fine fine to keep hwaudit_on
 within the control struct.

    Karel

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

Re: [PATCH 2/3] hwclock: remove audit from control struct

[J William Piggott]

On 08/30/2017 03:40 AM, Karel Zak wrote:
> On Tue, Aug 29, 2017 at 12:34:07PM -0400, J William Piggott wrote:
>>
>> hwaudit_on is only used in main() and hwclock_exit() and
>> that is unlikely to ever change.
> 
>  Hmm... the goal is minimize global variables to keep code readable.
>  It's strange to have a control struct and use global variable in the
>  same time.
> 
>> Remove it from the control struct because it:
>>  * cleans up an ugly directive in hwclock_exit
>>  * removes an argument from usage() and hwclock_exit()
>>  * makes usage() comply with util-linux standards
> 
>  I think usage() can call exit(EXIT_SUCCESS) and do not need play any
>  games with audit at all.

True for half of hwclock's functions. Perhaps this special exit
handling should only exist when we HAVE_LIBAUDIT?

Okay, I'll rethink this and update the pull request.

>  
>  On another places we have "ctl" so it's fine fine to keep hwaudit_on
>  within the control struct.
> 
>     Karel
> 

Re: [PATCH 2/3] hwclock: remove audit from control struct

[J William Piggott]

On 08/30/2017 01:56 PM, J William Piggott wrote:
> 
> 
> On 08/30/2017 03:40 AM, Karel Zak wrote:
>> On Tue, Aug 29, 2017 at 12:34:07PM -0400, J William Piggott wrote:
>>>
>>> hwaudit_on is only used in main() and hwclock_exit() and
>>> that is unlikely to ever change.
>>
>>  Hmm... the goal is minimize global variables to keep code readable.
>>  It's strange to have a control struct and use global variable in the
>>  same time.
>>
>>> Remove it from the control struct because it:
>>>  * cleans up an ugly directive in hwclock_exit
>>>  * removes an argument from usage() and hwclock_exit()
>>>  * makes usage() comply with util-linux standards
>>
>>  I think usage() can call exit(EXIT_SUCCESS) and do not need play any
>>  games with audit at all.
> 
> True for half of hwclock's functions. Perhaps this special exit
> handling should only exist when we HAVE_LIBAUDIT?
> 
> Okay, I'll rethink this and update the pull request.

So it seems that there are more problems with exit and audit handling
then I first thought. Improving them is going to be more invasive and
will need to wait for refactoring of main() and manipulate_clock().

For now I have implemented Karel's idea to mix the use of hwclock_exit()
and exit().

Pushed changes to the same branch:

The following changes since commit 58d57ae2d88ef638e3ff07c213b5158334d3f658:

  tests: update sfdisk wipe test (2017-09-01 10:42:51 +0200)

are available in the git repository at:

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

for you to fetch changes up to 5b8e46f7e7710e2bb88ff8e763997830c9494df2:

  hwclock: close hwaudit_fd unconditionally (2017-09-03 12:34:03 -0400)

----------------------------------------------------------------
J William Piggott (3):
      hwclock: remove sysexits.h
      hwclock: don't always use hwclock_exit
      hwclock: close hwaudit_fd unconditionally

 sys-utils/hwclock-rtc.c |  5 ++--
 sys-utils/hwclock.8.in  |  8 ++++++
 sys-utils/hwclock.c     | 76 ++++++++++++++++++-------------------------------
 3 files changed, 37 insertions(+), 52 deletions(-)

> 
>>  
>>  On another places we have "ctl" so it's fine fine to keep hwaudit_on
>>  within the control struct.
>>
>>     Karel
>>
> --
> 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 2/3] hwclock: remove audit from control struct

[Sami Kerola]

On 3 September 2017 at 20:59, J William Piggott <elseifthen@gmx.com> wrote:
> On 08/30/2017 01:56 PM, J William Piggott wrote:
>> On 08/30/2017 03:40 AM, Karel Zak wrote:
>>> On Tue, Aug 29, 2017 at 12:34:07PM -0400, J William Piggott wrote:
>>>>
>>>> hwaudit_on is only used in main() and hwclock_exit() and
>>>> that is unlikely to ever change.
>>>
>>>  Hmm... the goal is minimize global variables to keep code readable.
>>>  It's strange to have a control struct and use global variable in the
>>>  same time.
>>>
>>>> Remove it from the control struct because it:
>>>>  * cleans up an ugly directive in hwclock_exit
>>>>  * removes an argument from usage() and hwclock_exit()
>>>>  * makes usage() comply with util-linux standards
>>>
>>>  I think usage() can call exit(EXIT_SUCCESS) and do not need play any
>>>  games with audit at all.
>>
>> True for half of hwclock's functions. Perhaps this special exit
>> handling should only exist when we HAVE_LIBAUDIT?
>>
>> Okay, I'll rethink this and update the pull request.
>
> So it seems that there are more problems with exit and audit handling
> then I first thought. Improving them is going to be more invasive and
> will need to wait for refactoring of main() and manipulate_clock().
>
> For now I have implemented Karel's idea to mix the use of hwclock_exit()
> and exit().
>
> Pushed changes to the same branch:
>
> The following changes since commit 58d57ae2d88ef638e3ff07c213b5158334d3f658:
>
>   tests: update sfdisk wipe test (2017-09-01 10:42:51 +0200)
>
> are available in the git repository at:
>
>   git@github.com:jwpi/util-linux.git 170825
>
> for you to fetch changes up to 5b8e46f7e7710e2bb88ff8e763997830c9494df2:
>
>   hwclock: close hwaudit_fd unconditionally (2017-09-03 12:34:03 -0400)
>
> ----------------------------------------------------------------
> J William Piggott (3):
>       hwclock: remove sysexits.h
>       hwclock: don't always use hwclock_exit
>       hwclock: close hwaudit_fd unconditionally
>
>  sys-utils/hwclock-rtc.c |  5 ++--
>  sys-utils/hwclock.8.in  |  8 ++++++

Update to manual telling return zero is success and one is failure feels
unnecessary. I don't think any other manual page does that, and most
users know these values without being explicit. It's the unusual return
codes that we should document so that users has some idea what
could it mean  when a command returns 42.

>  sys-utils/hwclock.c     | 76 ++++++++++++++++++-------------------------------
>  3 files changed, 37 insertions(+), 52 deletions(-)
>
>>
>>>
>>>  On another places we have "ctl" so it's fine fine to keep hwaudit_on
>>>  within the control struct.

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

Re: [PATCH 2/3] hwclock: remove audit from control struct

[J William Piggott]

On 09/03/2017 05:54 PM, Sami Kerola wrote:
> On 3 September 2017 at 20:59, J William Piggott <elseifthen@gmx.com> wrote:
>> On 08/30/2017 01:56 PM, J William Piggott wrote:
>>> On 08/30/2017 03:40 AM, Karel Zak wrote:
>>>> On Tue, Aug 29, 2017 at 12:34:07PM -0400, J William Piggott wrote:
>>>>>
>>>>> hwaudit_on is only used in main() and hwclock_exit() and
>>>>> that is unlikely to ever change.
>>>>
>>>>  Hmm... the goal is minimize global variables to keep code readable.
>>>>  It's strange to have a control struct and use global variable in the
>>>>  same time.
>>>>
>>>>> Remove it from the control struct because it:
>>>>>  * cleans up an ugly directive in hwclock_exit
>>>>>  * removes an argument from usage() and hwclock_exit()
>>>>>  * makes usage() comply with util-linux standards
>>>>
>>>>  I think usage() can call exit(EXIT_SUCCESS) and do not need play any
>>>>  games with audit at all.
>>>
>>> True for half of hwclock's functions. Perhaps this special exit
>>> handling should only exist when we HAVE_LIBAUDIT?
>>>
>>> Okay, I'll rethink this and update the pull request.
>>
>> So it seems that there are more problems with exit and audit handling
>> then I first thought. Improving them is going to be more invasive and
>> will need to wait for refactoring of main() and manipulate_clock().
>>
>> For now I have implemented Karel's idea to mix the use of hwclock_exit()
>> and exit().
>>
>> Pushed changes to the same branch:
>>
>> The following changes since commit 58d57ae2d88ef638e3ff07c213b5158334d3f658:
>>
>>   tests: update sfdisk wipe test (2017-09-01 10:42:51 +0200)
>>
>> are available in the git repository at:
>>
>>   git@github.com:jwpi/util-linux.git 170825
>>
>> for you to fetch changes up to 5b8e46f7e7710e2bb88ff8e763997830c9494df2:
>>
>>   hwclock: close hwaudit_fd unconditionally (2017-09-03 12:34:03 -0400)
>>
>> ----------------------------------------------------------------
>> J William Piggott (3):
>>       hwclock: remove sysexits.h
>>       hwclock: don't always use hwclock_exit
>>       hwclock: close hwaudit_fd unconditionally
>>
>>  sys-utils/hwclock-rtc.c |  5 ++--
>>  sys-utils/hwclock.8.in  |  8 ++++++
> 
> Update to manual telling return zero is success and one is failure feels
> unnecessary. I don't think any other manual page does that, and most
> users know these values without being explicit. It's the unusual return
> codes that we should document so that users has some idea what
> could it mean  when a command returns 42.

* It is one and zero only for POSIX systems.
* Most users probably don't know that, programmers do.
* Athough it was undocumented hwclock was using sysexits.h so it is
  worth documenting the change for clarity.
* it doesn't hurt anything to document it.

> 
>>  sys-utils/hwclock.c     | 76 ++++++++++++++++++-------------------------------
>>  3 files changed, 37 insertions(+), 52 deletions(-)
>>
>>>
>>>>
>>>>  On another places we have "ctl" so it's fine fine to keep hwaudit_on
>>>>  within the control struct.
> 

[v2 PATCH 2/3] hwclock: don't always use hwclock_exit

[J William Piggott]

Special exit handling is not wanted for usage() or bad
command syntax. For example we do not want to audit:
hwclock --set --date foo

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

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 82f919048..e471fe1ae 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -1061,7 +1061,7 @@ static void out_version(void)
 }
 
 static void __attribute__((__noreturn__))
-usage(const struct hwclock_control *ctl)
+usage(void)
 {
 	fputs(USAGE_HEADER, stdout);
 	printf(_(" %s [function] [option...]\n"), program_invocation_short_name);
@@ -1105,7 +1105,7 @@ usage(const struct hwclock_control *ctl)
 	fputs(USAGE_SEPARATOR, stdout);
 	printf(USAGE_HELP_OPTIONS(22));
 	printf(USAGE_MAN_TAIL("hwclock(8)"));
-	hwclock_exit(ctl, EXIT_SUCCESS);
+	exit(EXIT_SUCCESS);
 }
 
 int main(int argc, char **argv)
@@ -1301,7 +1301,7 @@ int main(int argc, char **argv)
 			out_version();
 			return 0;
 		case 'h':			/* --help */
-			usage(&ctl);
+			usage();
 		default:
 			errtryhelp(EXIT_FAILURE);
 		}
@@ -1317,25 +1317,25 @@ int main(int argc, char **argv)
 
 	if (ctl.update && !ctl.set && !ctl.systohc) {
 		warnx(_("--update-drift requires --set or --systohc"));
-		hwclock_exit(&ctl, EXIT_FAILURE);
+		exit(EXIT_FAILURE);
 	}
 
 	if (ctl.noadjfile && !ctl.utc && !ctl.local_opt) {
 		warnx(_("With --noadjfile, you must specify "
 			"either --utc or --localtime"));
-		hwclock_exit(&ctl, EXIT_FAILURE);
+		exit(EXIT_FAILURE);
 	}
 
 	if (ctl.set || ctl.predict) {
 		if (!ctl.date_opt) {
 		warnx(_("--date is required for --set or --predict"));
-		hwclock_exit(&ctl, EXIT_FAILURE);
+		exit(EXIT_FAILURE);
 		}
 		if (parse_date(&when, ctl.date_opt, NULL))
 			set_time = when.tv_sec;
 		else {
 			warnx(_("invalid date '%s'"), ctl.date_opt);
-			hwclock_exit(&ctl, EXIT_FAILURE);
+			exit(EXIT_FAILURE);
 		}
 	}
 

[PATCH 3/3] hwclock: close hwaudit_fd unconditionally

[J William Piggott]

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

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 6f868df..81ede0a 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -1376,8 +1376,8 @@ hwclock_exit(int status)
 		audit_log_user_message(hwaudit_fd, AUDIT_USYS_CONFIG,
 				       "op=change-system-time", NULL, NULL, NULL,
 				       status);
-		close(hwaudit_fd);
 	}
+	close(hwaudit_fd);
 #endif
 	exit(status);
 }

Re: [PATCH 0/3] Pull Request

[Karel Zak]

On Tue, Aug 29, 2017 at 12:30:20PM -0400, J William Piggott wrote:
>   git@github.com:jwpi/util-linux.git 170825

Applied, thanks.

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