[PATCH 0/5] Pull Request

[PATCH 0/5] Pull Request

[J William Piggott]

The following changes since commit 8df545592d73b67b0bf119732b52a5e4c0662ec4:

  build: use --runstatedir instead of --localstatedir (2017-07-31 15:24:46 +0200)

are available in the git repository at:

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

for you to fetch changes up to b3e8105837505fc9b4637b4e41cca39eecb48ebf:

  hwclock: remove custom errno string (2017-07-31 16:10:47 -0400)

----------------------------------------------------------------
J William Piggott (5):
      hwclock: squash custom errno strings
      hwclock: squash custom errno strings
      hwclock: fix unimplemented ioctl test
      hwclock: remove custom errno string
      hwclock: remove custom errno string

 sys-utils/hwclock-cmos.c |  6 +-----
 sys-utils/hwclock-rtc.c  | 40 ++++++++++++++++------------------------
 sys-utils/hwclock.c      | 32 ++++++--------------------------
 sys-utils/hwclock.h      |  1 -
 4 files changed, 23 insertions(+), 56 deletions(-)

[PATCH 1/5] hwclock: squash custom errno strings

[J William Piggott]

hwclock previously used printf for custom errno messages.
Later they were converted to use warn(), but were not
squashed. One of the reasons for warn and errno is to avoid
making translators deal with a multitude custom strings.

Also the custom strings are incorrect:

hwclock --hctosys
hwclock: Must be superuser to set system clock.
Unable to set system clock.

We do not know what the system permissions are set to.  The
correct response is to simply say permission was denied; as
the default errno string does. The second line is redundant
and just adds noise the code and to logs.

Patched:

hwclock --hctosys
hwclock: settimeofday() failed: Operation not permitted

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

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index aadf196..c74e0e2 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -648,14 +648,8 @@ set_system_clock(const struct hwclock_control *ctl, const bool hclock_valid,
 			if (!rc)
 				rc = settimeofday(&newtime, &tz);
 			if (rc) {
-				if (errno == EPERM) {
-					warnx(_
-					      ("Must be superuser to set system clock."));
-					retcode = EX_NOPERM;
-				} else {
-					warn(_("settimeofday() failed"));
-					retcode = 1;
-				}
+				warn(_("settimeofday() failed"));
+				retcode = 1;
 			} else
 				retcode = 0;
 		}
@@ -744,14 +738,8 @@ static int set_system_clock_timezone(const struct hwclock_control *ctl)
 			rc = settimeofday(tv_null, &tz);
 
 		if (rc) {
-			if (errno == EPERM) {
-				warnx(_
-				      ("Must be superuser to set system clock."));
-				retcode = EX_NOPERM;
-			} else {
-				warn(_("settimeofday() failed"));
-				retcode = 1;
-			}
+			warn(_("settimeofday() failed"));
+			retcode = 1;
 		} else
 			retcode = 0;
 	}
@@ -1134,17 +1122,9 @@ manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
 			adjust_drift_factor(ctl, adjtime, nowtime,
 					    hclock_valid, hclocktime);
 	} else if (ctl->hctosys) {
-		rc = set_system_clock(ctl, hclock_valid, hclocktime);
-		if (rc) {
-			printf(_("Unable to set system clock.\n"));
-			return rc;
-		}
+		return set_system_clock(ctl, hclock_valid, hclocktime);
 	} else if (ctl->systz) {
-		rc = set_system_clock_timezone(ctl);
-		if (rc) {
-			printf(_("Unable to set system clock.\n"));
-			return rc;
-		}
+		return set_system_clock_timezone(ctl);
 	} else if (ctl->predict) {
 		hclocktime = time_inc(hclocktime, (double)
 				      -(tdrift.tv_sec + tdrift.tv_usec / 1E6));

[PATCH 2/5] hwclock: squash custom errno strings

[J William Piggott]

warn() appends ENOENT as: No such file or directory

The custom string is unnecessary and problematic for translators.

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

diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
index 2af2666..36ce397 100644
--- a/sys-utils/hwclock-rtc.c
+++ b/sys-utils/hwclock-rtc.c
@@ -402,10 +402,7 @@ int get_epoch_rtc(const struct hwclock_control *ctl, unsigned long *epoch_p)
 
 	rtc_fd = open_rtc(ctl);
 	if (rtc_fd < 0) {
-		if (errno == ENOENT)
-			warnx(_("%s does not exist."), rtc_dev_name);
-		else
-			warn(_("cannot open %s"), rtc_dev_name);
+		warn(_("cannot open %s"), rtc_dev_name);
 		return 1;
 	}
 
@@ -440,10 +437,7 @@ int set_epoch_rtc(const struct hwclock_control *ctl)
 
 	rtc_fd = open_rtc(ctl);
 	if (rtc_fd < 0) {
-		if (errno == ENOENT)
-			warnx(_("%s does not exist."), rtc_dev_name);
-		else
-			warn(_("cannot open %s"), rtc_dev_name);
+		warn(_("cannot open %s"), rtc_dev_name);
 		return 1;
 	}
 

[PATCH 3/5] hwclock: fix unimplemented ioctl test

[J William Piggott]

The rtc driver has not returned EINVAL for unimplemented ioctls since
v2.5.8 in 2002. However, it does return it for other errors; making the
current test potentially problematic. Since 9f3d0fc util-linux assumes
kernel >= 2.6.0 so remove EINVAL as an ioctl test.

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

diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
index 36ce397..ea8357d 100644
--- a/sys-utils/hwclock-rtc.c
+++ b/sys-utils/hwclock-rtc.c
@@ -257,7 +257,7 @@ static int synchronize_to_clock_tick_rtc(const struct hwclock_control *ctl)
 		 * they should.
 		 */
 		rc = -1;
-		errno = EINVAL;
+		errno = ENOTTY;
 #else
 		rc = ioctl(rtc_fd, RTC_UIE_ON, 0);
 #endif
@@ -293,7 +293,7 @@ static int synchronize_to_clock_tick_rtc(const struct hwclock_control *ctl)
 			if (rc == -1)
 				warn(_("ioctl() to %s to turn off update interrupts failed"),
 				     rtc_dev_name);
-		} else if (errno == ENOTTY || errno == EINVAL) {
+		} else if (errno == ENOTTY) {
 			/*
 			 * This rtc device doesn't have interrupt functions.
 			 * This is typical on an Alpha, where the Hardware

[PATCH 4/5] hwclock: remove custom errno string

[J William Piggott]

Custom errno messages are unnecessary and problematic for translators.

The current messages are also too long, > 90 columns:
/dev/rtc0 does not have interrupt functions. Waiting in loop for time from \
/dev/rtc0 to change

Fixed:
ioctl(3, RTC_UIE_ON, 0): Inappropriate ioctl for device
Waiting in loop for time from /dev/rtc0 to change

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

diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
index ea8357d..c738076 100644
--- a/sys-utils/hwclock-rtc.c
+++ b/sys-utils/hwclock-rtc.c
@@ -194,8 +194,11 @@ static int do_rtc_read_ioctl(int rtc_fd, struct tm *tm)
 }
 
 /*
- * Wait for the top of a clock tick by reading /dev/rtc in a busy loop until
- * we see it.
+ * Wait for the top of a clock tick by reading /dev/rtc in a busy loop
+ * until we see it. This function is used for rtc drivers without ioctl
+ * interrupts. This is typical on an Alpha, where the Hardware Clock
+ * interrupts are used by the kernel for the system clock, so aren't at
+ * the user's disposal.
  */
 static int busywait_for_rtc_clock_tick(const struct hwclock_control *ctl,
 				       const int rtc_fd)
@@ -206,9 +209,12 @@ static int busywait_for_rtc_clock_tick(const struct hwclock_control *ctl,
 	int rc;
 	struct timeval begin, now;
 
-	if (ctl->debug)
+	if (ctl->debug) {
+		printf("ioctl(%d, RTC_UIE_ON, 0): %s\n",
+		       rtc_fd, strerror(errno));
 		printf(_("Waiting in loop for time from %s to change\n"),
 		       rtc_dev_name);
+	}
 
 	rc = do_rtc_read_ioctl(rtc_fd, &start_time);
 	if (rc)
@@ -294,19 +300,11 @@ static int synchronize_to_clock_tick_rtc(const struct hwclock_control *ctl)
 				warn(_("ioctl() to %s to turn off update interrupts failed"),
 				     rtc_dev_name);
 		} else if (errno == ENOTTY) {
-			/*
-			 * This rtc device doesn't have interrupt functions.
-			 * This is typical on an Alpha, where the Hardware
-			 * Clock interrupts are used by the kernel for the
-			 * system clock, so aren't at the user's disposal.
-			 */
-			if (ctl->debug)
-				printf(_("%s does not have interrupt functions. "),
-				       rtc_dev_name);
+			/* rtc ioctl interrupts are unimplemented */
 			ret = busywait_for_rtc_clock_tick(ctl, rtc_fd);
 		} else
-			warn(_("ioctl() to %s to turn on update interrupts "
-			      "failed unexpectedly"), rtc_dev_name);
+			warn(_("ioctl(%d, RTC_UIE_ON, 0) to %s failed"),
+			     rtc_fd, rtc_dev_name);
 	}
 	return ret;
 }

[PATCH 5/5] hwclock: remove custom errno string

[J William Piggott]

Custom errno messages are unnecessary and problematic for translators.

hwclock --directisa
hwclock: iopl() port access failed: Operation not permitted
hwclock: root privileges may be required

The custom errno message is misleading. We do not know what
the system permissions are set to. The default errno string is
correct, and enough.

Patched:
hwclock --directisa
hwclock: iopl() port access failed: Operation not permitted

root@:~# hwclock --directisa -D
Using direct ISA access to the clock
2017-07-24 14:49:17.782716-0400

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

diff --git a/sys-utils/hwclock-cmos.c b/sys-utils/hwclock-cmos.c
index 6227b94..cdbc88b 100644
--- a/sys-utils/hwclock-cmos.c
+++ b/sys-utils/hwclock-cmos.c
@@ -45,7 +45,6 @@
  *   tm_isdst	>0: yes, 0: no, <0: unknown
  */
 
-#include <errno.h>
 #include <fcntl.h>
 #include <stdio.h>
 #include <string.h>
@@ -390,12 +389,9 @@ static int get_permissions_cmos(void)
 	if (rc == IOPL_NOT_IMPLEMENTED) {
 		warnx(_("ISA port access is not implemented"));
 	} else if (rc != 0) {
-		rc = errno;
 		warn(_("iopl() port access failed"));
-		if (rc == EPERM && geteuid())
-			warnx(_("root privileges may be required"));
 	}
-	return rc ? 1 : 0;
+	return rc;
 }
 
 static struct clock_ops cmos_interface = {
diff --git a/sys-utils/hwclock.h b/sys-utils/hwclock.h
index a1ef80c..8843501 100644
--- a/sys-utils/hwclock.h
+++ b/sys-utils/hwclock.h
@@ -1,7 +1,6 @@
 #ifndef HWCLOCK_CLOCK_H
 #define HWCLOCK_CLOCK_H
 
-#include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>

Re: [PATCH 0/5] Pull Request

[Karel Zak]

On Mon, Jul 31, 2017 at 07:36:22PM -0400, J William Piggott wrote:
>       hwclock: squash custom errno strings
>       hwclock: squash custom errno strings
>       hwclock: fix unimplemented ioctl test
>       hwclock: remove custom errno string
>       hwclock: remove custom errno string

 Cool, I like it a lot. This is exactly how error/warning messages
 should be composed. Don't guess anything, don't try to explain
 everything, etc ... just print errno based string.

 Applied, thanks.

    Karel

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

Re: [PATCH 0/5] Pull Request

[J William Piggott]

On 08/01/2017 05:51 AM, Karel Zak wrote:
> On Mon, Jul 31, 2017 at 07:36:22PM -0400, J William Piggott wrote:
>>       hwclock: squash custom errno strings
>>       hwclock: squash custom errno strings
>>       hwclock: fix unimplemented ioctl test
>>       hwclock: remove custom errno string
>>       hwclock: remove custom errno string
> 
>  Cool, I like it a lot. This is exactly how error/warning messages
>  should be composed. Don't guess anything, don't try to explain
>  everything, etc ... just print errno based string.

Thanks for the feedback. I feel the same. hwclock messages editorialize
and are too verbose. There are more to fix on my todo list. The --systz
function has the "Don't guess anything" problem. It prints what it
expects the kernel to do as being fact.


> 
>  Applied, thanks.
> 
>     Karel
> 

[PATCH 0/5] pull request

[J William Piggott]

The following changes since commit 07fd6640c80ffd7a2067844aadcb6162431b3525:

  Merge branch 'path-fixes' of https://github.com/rudimeier/util-linux (2017-06-29 15:29:33 +0200)

are available in the git repository at:

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

for you to fetch changes up to 9c50e384c89d5f9391c3eaed8df89ec965f2fda7:

  hwclock: sync one-liner descriptions (2017-07-02 14:09:41 -0400)

----------------------------------------------------------------
J William Piggott (5):
      hwclock: remove from usage() FILE *out = stdout
      hwclock: usage() use program_invocation_short_name
      hwclock: final usage() strings slice
      docs: update boilerplate.c usage()
      hwclock: sync one-liner descriptions

 Documentation/boilerplate.c | 37 ++++++++++++------------
 sys-utils/hwclock.8.in      |  4 +--
 sys-utils/hwclock.c         | 68 +++++++++++++++++++++++----------------------
 3 files changed, 55 insertions(+), 54 deletions(-)