Pull Request

PATCH Applied J William Piggott 3 participants 11 messages Aug 29, 2017 – Sep 5, 2017 Util-Linux package

lore

Share & tools

lore

J William Piggott

author

2017-08-29 16:30 UTC #0 lore raw

cover Pull Request

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:

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

J William Piggott

author

2017-08-29 16:33 UTC #1 lore raw

PATCH 1/3 hwclock: remove sysexits.h

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

---
 sys-utils/hwclock-rtc.c |  5 ++--
 sys-utils/hwclock.8.in  |  8 ++++++
 sys-utils/hwclock.c     | 68 +++++++++++++++++--------------------------------
 3 files changed, 33 insertions(+), 48 deletions(-)

sys-utils/hwclock-rtc.c +2 -3

index c50011a..62e4329 100644
@@ -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)

sys-utils/hwclock.8.in +8 -0

index 107e3f1..8bdc841 100644
@@ -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

sys-utils/hwclock.c +23 ~~-45~~

203 lines not on this page · raw message

J William Piggott

author

2017-08-29 16:34 UTC #2 lore raw

PATCH 2/3 hwclock: remove audit from control struct

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

---
 sys-utils/hwclock-rtc.c |  4 ++--
 sys-utils/hwclock.c     | 46 +++++++++++++++++++++++-----------------------
 sys-utils/hwclock.h     |  3 +--
 3 files changed, 26 insertions(+), 27 deletions(-)

sys-utils/hwclock-rtc.c +2 -2

index 62e4329..9d52e40 100644
@@ -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)

sys-utils/hwclock.c +23 ~~-23~~

159 lines not on this page · raw message

sys-utils/hwclock.h +1 -2

index 6943d8d..013a7a3 100644
@@ -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 */

Karel Zak

maintainer

2017-08-30 07:40 UTC #3 lore raw

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

Quoted 3 lines · J William Piggott

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.

Quoted 4 lines

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

Signature · 2 lines

 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com

J William Piggott

author

2017-08-30 17:56 UTC #4 lore raw

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

Quoted 9 lines · Karel Zak

Quoted 3 lines · J William Piggott
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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 6 lines

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

J William Piggott

author

2017-09-03 19:59 UTC #5 lore raw

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

Quoted 17 lines · J William Piggott

Quoted 14 lines · Karel Zak
Quoted 3 lines · J William Piggott
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.
Quoted 4 lines
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

Quoted 7 lines

Quoted 2 lines
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:

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

Quoted 12 lines

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

Sami Kerola

2017-09-03 21:54 UTC #6 lore raw

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

Quoted 45 lines · J William Piggott

Quoted 15 lines · J William Piggott
Quoted 14 lines · Karel Zak
Quoted 3 lines · J William Piggott
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.
Quoted 4 lines
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
Quoted 7 lines
Quoted 2 lines
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:
[email protected]: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)

Quoted 8 lines

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

Quoted 7 lines

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

Signature · 2 lines

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

J William Piggott

author

2017-09-04 01:22 UTC #7 lore raw

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

Quoted 52 lines · Sami Kerola

Quoted 51 lines · J William Piggott
Quoted 15 lines · J William Piggott
Quoted 14 lines · Karel Zak
Quoted 3 lines · J William Piggott
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.
Quoted 4 lines
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
Quoted 7 lines
Quoted 2 lines
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:
[email protected]: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

Quoted 8 lines

Quoted 2 lines
 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.

Quoted 9 lines

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

J William Piggott

author

2017-09-03 20:02 UTC #8 lore raw

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

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

---
 sys-utils/hwclock.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

sys-utils/hwclock.c +7 -7

index 82f919048..e471fe1ae 100644
@@ -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);
 		}
 	}

J William Piggott

author

2017-08-29 16:35 UTC #9 lore raw

PATCH 3/3 hwclock: close hwaudit_fd unconditionally

In reply to J William Piggott

Signed-off-by J William Piggott <[email protected]>

---
 sys-utils/hwclock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

sys-utils/hwclock.c +1 -1

index 6f868df..81ede0a 100644
@@ -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);
 }

Karel Zak

maintainer Applied

2017-09-05 10:27 UTC #10 lore raw

In reply to J William Piggott

Quoted 1 line · J William Piggott

[email protected]:jwpi/util-linux.git 170825

Applied, thanks.

Signature · 2 lines

 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com