* pull: hwclock 27 changes
@ 2016-12-31 21:41 Sami Kerola
2017-01-03 14:34 ` J William Piggott
[not found] ` <05e4405f-8096-9261-f9f2-d2c6b84675bc@gmx.com>
0 siblings, 2 replies; 28+ messages in thread
From: Sami Kerola @ 2016-12-31 21:41 UTC (permalink / raw)
To: util-linux; +Cc: Sami Kerola
Good New Year,
Most of the changes in this pull request has been lying in my git repo for
few months, and now when 2016 is coming to an end I had a look what
unsubmitted work there is. After reviewing my changes, and improving some
of them modestly, these are now ready for review.
----------------------------------------------------------------
The following changes since commit 963f7dfb82d44661d76c6fb61fda309b846726ce:
Merge branch 'getrandom' of git://github.com/kerolasa/lelux-utiliteetit (2016-12-22 15:27:25 +0100)
are available in the git repository at:
git://github.com/kerolasa/lelux-utiliteetit.git hwclock
for you to fetch changes up to c7a8f56920c738a90038cd6baaaec676c642b926:
hwclock: remove trailing dot from messages that include system error message (2016-12-31 21:29:27 +0000)
----------------------------------------------------------------
Sami Kerola (27):
hwclock: remove UTC-0 localization hack
lib: add timegm() portability function to lib/timeutils.c
hwclock: remove FLOOR macro
hwclock: remove hwclock_exit() indirection
hwclock: do not hardcode date command magic string twice
hwclock: remove unnecessary type casts
hwclock: move command-line options to control structure
hwclock: clarify set_cmos_epoch() code
hwclock: move error messages to determine_clock_access_method()
docs: add instructions how to avoid alpha epoc Y2020 bug
hwclock: remove dead code and other minor fixes
hwclock: simplify save_adjtime() execution flow
hwclock: alloate date_resp parsing buffer in interpret_date_string()
hwclock: initialize struct adjtime members
hwclock: use symbolic magic values passed in between functions
hwclock: remove magic constants from interpret_date_string()
hwclock: add debugging to open_rtc()
hwclock: remove division by zero [asan]
hwclock: use integer arithmetics in compare_clock()
docs: deprecate hwclock --compare option
hwclock: improve coding style
hwclock: stream line synchronize_to_clock_tick_rtc()
hwclock: try RTCGET and RTCSET only when normal rtc fails
hwclock: clarify cmos inb and outb preprocessor directives
hwclock: fix rtc atexit registration
hwclock: make --date=argument less prone to injection
hwclock: remove trailing dot from messages that include system error message
configure.ac | 1 +
include/pathnames.h | 4 +
include/timeutils.h | 4 +
lib/timeutils.c | 19 ++
login-utils/utmpdump.c | 1 +
sys-utils/hwclock-cmos.c | 228 +++++++------
sys-utils/hwclock-rtc.c | 221 ++++++------
sys-utils/hwclock.8.in | 11 +
sys-utils/hwclock.c | 874 +++++++++++++++++++++--------------------------
sys-utils/hwclock.h | 71 +++-
10 files changed, 702 insertions(+), 732 deletions(-)
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2016-12-31 21:41 pull: hwclock 27 changes Sami Kerola
@ 2017-01-03 14:34 ` J William Piggott
[not found] ` <05e4405f-8096-9261-f9f2-d2c6b84675bc@gmx.com>
1 sibling, 0 replies; 28+ messages in thread
From: J William Piggott @ 2017-01-03 14:34 UTC (permalink / raw)
To: Sami Kerola, util-linux
Hey Sami,
I'm reviewing these, but there is a lot to digest.
Karel, I hope you are not in a hurry to commit these. I should have a
review ready in a few days.
On 12/31/2016 04:41 PM, Sami Kerola wrote:
> Good New Year,
>
> Most of the changes in this pull request has been lying in my git repo for
> few months, and now when 2016 is coming to an end I had a look what
> unsubmitted work there is. After reviewing my changes, and improving some
> of them modestly, these are now ready for review.
>
> ----------------------------------------------------------------
>
> The following changes since commit 963f7dfb82d44661d76c6fb61fda309b846726ce:
>
> Merge branch 'getrandom' of git://github.com/kerolasa/lelux-utiliteetit (2016-12-22 15:27:25 +0100)
>
> are available in the git repository at:
>
> git://github.com/kerolasa/lelux-utiliteetit.git hwclock
>
> for you to fetch changes up to c7a8f56920c738a90038cd6baaaec676c642b926:
>
> hwclock: remove trailing dot from messages that include system error message (2016-12-31 21:29:27 +0000)
>
> ----------------------------------------------------------------
> Sami Kerola (27):
> hwclock: remove UTC-0 localization hack
> lib: add timegm() portability function to lib/timeutils.c
> hwclock: remove FLOOR macro
> hwclock: remove hwclock_exit() indirection
> hwclock: do not hardcode date command magic string twice
> hwclock: remove unnecessary type casts
> hwclock: move command-line options to control structure
> hwclock: clarify set_cmos_epoch() code
> hwclock: move error messages to determine_clock_access_method()
> docs: add instructions how to avoid alpha epoc Y2020 bug
> hwclock: remove dead code and other minor fixes
> hwclock: simplify save_adjtime() execution flow
> hwclock: alloate date_resp parsing buffer in interpret_date_string()
> hwclock: initialize struct adjtime members
> hwclock: use symbolic magic values passed in between functions
> hwclock: remove magic constants from interpret_date_string()
> hwclock: add debugging to open_rtc()
> hwclock: remove division by zero [asan]
> hwclock: use integer arithmetics in compare_clock()
> docs: deprecate hwclock --compare option
> hwclock: improve coding style
> hwclock: stream line synchronize_to_clock_tick_rtc()
> hwclock: try RTCGET and RTCSET only when normal rtc fails
> hwclock: clarify cmos inb and outb preprocessor directives
> hwclock: fix rtc atexit registration
> hwclock: make --date=argument less prone to injection
> hwclock: remove trailing dot from messages that include system error message
>
> configure.ac | 1 +
> include/pathnames.h | 4 +
> include/timeutils.h | 4 +
> lib/timeutils.c | 19 ++
> login-utils/utmpdump.c | 1 +
> sys-utils/hwclock-cmos.c | 228 +++++++------
> sys-utils/hwclock-rtc.c | 221 ++++++------
> sys-utils/hwclock.8.in | 11 +
> sys-utils/hwclock.c | 874 +++++++++++++++++++++--------------------------
> sys-utils/hwclock.h | 71 +++-
> 10 files changed, 702 insertions(+), 732 deletions(-)
> --
> 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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
[not found] ` <05e4405f-8096-9261-f9f2-d2c6b84675bc@gmx.com>
@ 2017-01-07 19:37 ` J William Piggott
2017-01-07 20:32 ` J William Piggott
2017-01-07 23:06 ` Sami Kerola
0 siblings, 2 replies; 28+ messages in thread
From: J William Piggott @ 2017-01-07 19:37 UTC (permalink / raw)
To: Sami Kerola, util-linux
Below is a monolithic review of the pull request using reverse quoting
so the code is easier to view/highlight.
Edit: I sent this once and it choked somewhere. So below is highly
redacted version.
---
commit c7a8f56920c738a90038cd6baaaec676c642b926
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sat Dec 31 21:29:27 2016 +0000
hwclock: remove trailing dot from messages that include system error message
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> Nice, thank you.
>
> FWIW, I think this should apply to all message strings. (Apparently these
> types of changes annoy translators :)
>
commit 3d3444cfc24c843e9efc65a56e78f8060d438a7c
Author: Sami Kerola <kerolasa@iki.fi>
Date: Wed Jul 27 19:47:38 2016 +0100
hwclock: make --date=argument less prone to injection
This change should not improve security much. One hopes hwclock --set is
restricted for root only. Where hwclock is allowed to run via sudo, or has
setuid setup, there is a pretty easy privilege escalation via subshell.
$ sudo ./hwclock --set --date='2000-10-20$(touch /tmp/hwclock.inject)'
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 9a5cb8f..2c92fbf 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -671,10 +671,12 @@ static int interpret_date_string(const struct hwclock_control *ctl, time_t * con
}
/* Quotes in date_opt would ruin the date command we construct. */
- if (strchr(ctl->date_opt, '"') != NULL) {
+ if (strchr(ctl->date_opt, '"') != NULL ||
+ strchr(ctl->date_opt, '`') != NULL ||
+ strstr(ctl->date_opt, "$(") != NULL) {
warnx(_
("The value of the --date option is not a valid date.\n"
- "In particular, it contains quotation marks."));
+ "In particular, it contains illegal character(s)."));
return retcode;
}
>
> Nice, thank you. I think just $ would have been enough.
>
commit fec97d35eb42601a347d5cac7ddd5b1d422edbbe
Author: Sami Kerola <kerolasa@iki.fi>
Date: Tue Jul 26 14:34:30 2016 +0100
hwclock: fix rtc atexit registration
Commit 27f9db17bd57b85947445c03e2cd9dda36ca377f missed a minus sign from
comparison.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
index 9a67027..7e1a945 100644
--- a/sys-utils/hwclock-rtc.c
+++ b/sys-utils/hwclock-rtc.c
@@ -138,7 +138,7 @@ static int open_rtc(const struct hwclock_control *ctl)
if (rtc_dev_fd < 0)
rtc_dev_name = *fls; /* default for error messages */
}
- if (rtc_dev_fd != 1)
+ if (rtc_dev_fd != -1)
atexit(close_rtc);
return rtc_dev_fd;
}
>
> Nice catch, thank you.
>
commit 6f62eca023c0ec187a369cc668a962c696fd1107
Author: Sami Kerola <kerolasa@iki.fi>
Date: Tue Jul 26 08:33:32 2016 +0100
hwclock: clarify cmos inb and outb preprocessor directives
The cmos only works when architecture is i386, x86_64, or alpha.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> Nice, thank you.
>
commit d9ccb70cafaac62fd6c8294fbba3b3ab385d3537
Author: Sami Kerola <kerolasa@iki.fi>
Date: Tue Jul 26 10:52:15 2016 +0100
hwclock: try RTCGET and RTCSET only when normal rtc fails
The RTCGET and RTCSET are in use for sparcs with sbus, so try them as
fallback rather than always.
Reference: https://github.com/torvalds/linux/blob/master/fs/compat_ioctl.c#L967-L974
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
index fc1dfc3..9a67027 100644
--- a/sys-utils/hwclock-rtc.c
+++ b/sys-utils/hwclock-rtc.c
@@ -158,28 +158,31 @@ static int do_rtc_read_ioctl(int rtc_fd, struct tm *tm)
{
int rc = -1;
char *ioctlname;
-
#ifdef __sparc__
/* some but not all sparcs use a different ioctl and struct */
struct sparc_rtc_time stm;
+#endif
8< -------
>
> Can't this be in the sparc ifdef just after this?
>
> Otherwise looks OK, thank you.
>
commit 08e959db08840871979b00036d85aa6941f9fa76
Author: Sami Kerola <kerolasa@iki.fi>
Date: Tue Jul 26 08:54:00 2016 +0100
hwclock: stream line synchronize_to_clock_tick_rtc()
Flip if clauses to hit common case first. This should be easier and quicker
to read and run.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
index f54e263..fc1dfc3 100644
--- a/sys-utils/hwclock-rtc.c
+++ b/sys-utils/hwclock-rtc.c
8< -------
@@ -287,26 +276,34 @@ static int synchronize_to_clock_tick_rtc(const struct hwclock_control *ctl)
tv.tv_sec = 10;
tv.tv_usec = 0;
rc = select(rtc_fd + 1, &rfds, NULL, NULL, &tv);
- ret = 1;
- if (rc == -1)
- warn(_("select() to %s to wait for clock tick failed"),
- rtc_dev_name);
+ if (0 < rc)
+ ret = 0;
else if (rc == 0) {
if (ctl->debug)
printf(_("select() to %s to wait for clock tick timed out"),
rtc_dev_name);
} else
- ret = 0;
+ warn(_("select() to %s to wait for clock tick failed"),
+ rtc_dev_name);
/* Turn off update interrupts */
rc = ioctl(rtc_fd, RTC_UIE_OFF, 0);
if (rc == -1)
warn(_("ioctl() to %s to turn off update interrupts failed"),
rtc_dev_name);
>
>
> The /* Turn off update interrupts */ code was the success block. You
> didn't move it up to your new success block at 'if (0 < rc)'. The
> interrupts will never be turned off with this patch.
>
>
8< -------
commit bb08719b0160d560defe091e60687d5d1240e88c
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sun Jul 24 22:33:01 2016 +0100
hwclock: improve coding style
Make string constants to be symbolical declarations. Use longer variable
name for rtc and cmos function pointer values. Exclude code that is
architecture specific with preprocessor directives. And remove message
duplication.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> Nice, thank you.
>
commit 80b6f08f52e21205f41de0a51a26abd4034cf0cf
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sat Jul 23 12:20:03 2016 +0100
docs: deprecate hwclock --compare option
Earlier commit clarified arithmetics that implement hwclock --compare
option, and that functionality feels like rubbish in - rubbish out. This
surely cannot be correct way to find frequency offset and/or tick. Even if
in principle doing calculations this way makes sense, the measurements are
prone to get skew due scheduling and other environment effects. Therefore
mark this option deprecated.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
diff --git a/sys-utils/hwclock.8.in b/sys-utils/hwclock.8.in
index de68e43..58b5158 100644
--- a/sys-utils/hwclock.8.in
+++ b/sys-utils/hwclock.8.in
@@ -45,6 +45,9 @@ discussion below, under
Periodically compare the Hardware Clock to the System Time and output
the difference every 10 seconds. This will also print the frequency
offset and tick.
+.IP
+The compare output is not a good measurement of offset or tick.
+This functionality is deprecated, and will be removed in future.
.
.TP
.B \-\-getepoch
diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 774701f..9a5cb8f 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -1385,6 +1385,9 @@ static inline time_t get_times(const struct hwclock_control *ctl,
/*
* Compare the system and CMOS time and output the drift
* in 10 second intervals.
+ *
+ * FIXME: remove this function, unused functions, related option parsing,
+ * and manual entry when ever after 2019-07-01.
*/
static int compare_clock(const struct hwclock_control *ctl)
{
>
>
> I tried to convince Karel of this long ago. This code has been broken
> from day one, so nobody can be using it. IMO, it should be removed now.
>
>
commit 2c2bfd4828ced9f3ca1f0288d6447a0bd8f62bba
Author: Sami Kerola <kerolasa@iki.fi>
Date: Fri Jul 22 22:45:35 2016 +0100
hwclock: use integer arithmetics in compare_clock()
Almost all floating point arithmetics are now converted to interger
arithmetics in compare_clock(). In same go variables names are renamed to
be hopefully a little more easier to understand. The second time
substraction got new comment because it took a while to understand out what
the code tried to do.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
8< -------
>
> Not reviewing. It's pointless as --compare is still broken.
>
>
commit 1ab5409350480e7eb1b4c063eae75f8bb35d6bfe
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sun Jul 17 22:04:32 2016 +0100
hwclock: remove division by zero [asan]
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
8< -------
>
> Not reviewing.
>
>
commit 8a0279382b4be44a173a8e9419a7fd211fcb1443
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sun Jul 17 17:10:48 2016 +0100
hwclock: add debugging to open_rtc()
Earlier when open_rtc() returned -1 the char *rtc_dev_name end up having
NULL that made it unsuitable to be used in error message. Now one can debug
what paths the open_rtc() tries to use when one has to debug why 'cannot
open rtc device' happen.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
8< -------
>
> Nice, thank you.
>
>
commit 4c4965c0e98da9e6b4e64de4c1dff7d41eb81018
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sun Jul 17 16:35:40 2016 +0100
hwclock: remove magic constants from interpret_date_string()
The constants function returned were not used. In same go clean up
execution flow a little bit.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
8< -------
>
> Nice, thank you.
>
commit 1d3829c027cb5e09e698b803a42a523d49f7815f
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sun Jul 17 16:14:16 2016 +0100
hwclock: use symbolic magic values passed in between functions
The manipulate_clock() is seeing return value from
busywait_for_rtc_clock_tick().
And the get_permissions_cmos() can see i386_iopl() return value.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
8< -------
>
> Nice, thank you.
>
>
commit 017de7d8231aaaf8cd57fa9b0887e949c9c4c683
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sun Jul 17 13:25:40 2016 +0100
hwclock: initialize struct adjtime members
Avoid any change of using uninitialized values. By looks of it the earlier
code did take care of that, but was less obvious about the fact.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> s/change/chance/
>
> Above is the important change, but as long as you're fixing that:
>
> s/By looks of it/It looks like/
> s/but/but it/
8< -------
>
> Otherwise nice, thank you.
>
commit c4e533646fef70dbd8ae2bbc2e3becb1954b5353
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sun Jul 17 12:49:12 2016 +0100
hwclock: alloate date_resp parsing buffer in interpret_date_string()
This makes overflowing the variable in question impossible.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
8< -------
>
> Nice, thank you.
>
commit 573a993964bd695b68c62a9eb75fdd312ad3f836
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sun Jul 17 12:12:52 2016 +0100
hwclock: simplify save_adjtime() execution flow
Return early to avoid excessive nesting. In same go remove any change of
overflow by using appropriate allocation. And update variable names to be
easier to understand.
>
> s/change/chance/
>
8< -------
>
> Otherwise looks OK, thank you.
>
commit 285ac4c3899d0876f53591962ac5119607d71474
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sun Jul 17 11:30:40 2016 +0100
hwclock: remove dead code and other minor fixes
Use #ifdef rather than #if to avoid undefined preprocessor identifier
warning.
Remove dead code. The #if 0 ensured the code has not been used for long
time, which is good because the linux/mc146818rtc.h is not been part of
user-api for long time.
Value of the adjtime_p->last_calib_time is checked if it has value of zero,
so testing none-zero bit later is necessarily true, and therefore does not
need to be checked.
And at the and remove unnecessary boolean variable.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
8< -------
>
> Looks OK, thank you.
>
commit 509ac719dbe1bcc4a761f694497493525da39e25
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sun Jul 17 09:09:33 2016 +0100
docs: add instructions how to avoid alpha epoc Y2020 bug
Recommend setting epoch using command line to for Alpha to avoid
autodetection that is doomed to afail after 2020.
Reference: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/alpha/kernel/rtc.c?id=85d0b3a573d8b711ee0c96199ac24a0f3283ed68
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
diff --git a/sys-utils/hwclock-cmos.c b/sys-utils/hwclock-cmos.c
index 9d9a42a..cf26064 100644
--- a/sys-utils/hwclock-cmos.c
+++ b/sys-utils/hwclock-cmos.c
@@ -203,15 +203,16 @@ void set_cmos_epoch(const struct hwclock_control *ctl)
#endif
/*
- * The kernel source today says: read the year.
+ * The automatic epoc determination in kernel
+ * linux/arch/alpha/kernel/rtc.c
*
* If it is in 0-19 then the epoch is 2000.
* If it is in 20-47 then the epoch is 1980.
* If it is in 48-69 then the epoch is 1952.
* If it is in 70-99 then the epoch is 1928.
*
- * Otherwise the epoch is 1900.
- * TODO: Clearly, this must be changed before 2019.
+ * Alpha users should explicitly define epoc using rtc kernel module
+ * option rtc epoc=<year>
*/
/*
* See whether we are dealing with SRM or MILO, as they have
diff --git a/sys-utils/hwclock.8.in b/sys-utils/hwclock.8.in
index 9534c19..de68e43 100644
--- a/sys-utils/hwclock.8.in
+++ b/sys-utils/hwclock.8.in
@@ -466,6 +466,14 @@ For example, on a Digital Unix machine:
.IP "" 4
.B hwclock\ \-\-setepoch\ \-\-epoch=1952
.RE
+.IP
+Beginning January 1, 2020 automatic epoc determination in kernel for systems
+using 1900 will fail. In these cases it is recommended to specify epoc
+explicitly with kernel module option.
+.RS
+.IP "" 4
+.B options rtc epoch=1900
+.RE
.
.TP
.B \-\-funky\-toy
>
> I don't like this one because:
>
> Despite Richard's "doomed to fail" comment, I don't think util-linux
> should document that as a fact. The heuristics could be updated. If the
> kernel is still supporting DEC in 2019 it seems like they should
> update them. It would be more work to remove the code than to adjust
> the numbers.
>
> Also the patch has some errors:
>
> * The possible failure applies to more than 'systems using 1900'.
>
> * The Alpha rtc driver cannot be built as a module; I believe
> epoch={year} is a kernel command line argument only.
>
> * The heuristics in drivers/char/rtc.c are slightly different than
> what the patch references in linux/arch/alpha/kernel/rtc.c. I don't
> know which one wins. They both differ from how they are described in
> the hwclock-cmos.c comments, from which this patch deletes the 1900
> possibility.
>
> The Alpha auto epoch detect has nothing to do with hwclock --setepoch
> --epoch {year}. That is all done via rtc and can only set it manually.
> Auto detecting the epoch only affects using --directisa on Alpha
> machines. Direct access requires hwclock to handle the epoch offset
> internally, because we're bypassing the kernel. So hwclock-cmos.c has
> to figure out what to use. It tries heuristics and, ironically, it
> tries to use the kernel's auto detect via the rtc driver. Why use
> direct access if you have a working rtc interface!
>
> It is undocumented, but when using --directisa on alpha --epoch {year}
> can be used to bypass auto detect. So --epoch {year} would be used for
> all functions. The man-page says it can only be used with --setepoch.
> I have no way to test this, so until now I have not documented it.
>
commit 6deb1326df19a87a97a5e77550f87c31a0367c92
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sat Jul 16 22:15:54 2016 +0100
hwclock: move error messages to determine_clock_access_method()
This makes main() a little bit shorter.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
8< -------
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -1189,17 +1189,24 @@ static void determine_clock_access_method(const struct hwclock_control *ctl)
if (ctl->directisa)
ur = probe_for_cmos_clock();
-
#ifdef __linux__
if (!ur)
ur = probe_for_rtc_clock(ctl);
#endif
+ if (ur) {
+ if (ctl->debug)
+ puts(ur->interface_name);
- if (ctl->debug) {
- if (ur)
- puts(_(ur->interface_name));
- else
+ } else {
+ if (ctl->debug)
printf(_("No usable clock interface found.\n"));
+ warnx(_("Cannot access the Hardware Clock via "
+ "any known method."));
>
> warnx( indentation is wrong.
>
8< -------
>
> Otherwise looks OK, thank you.
>
commit bf8f8287b095f9b1401253ecea46bab5ac96d811
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sat Jul 16 20:18:19 2016 +0100
hwclock: clarify set_cmos_epoch() code
Variable set_epoc is unnecessary, and removal of it makes it obvious what is
happening in this function.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
8< -------
>
> Looks OK, thank you.
>
commit 59aff057349335cd089c34bb0eaa972cf60ff96a
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sat Jul 16 16:45:07 2016 +0100
hwclock: move command-line options to control structure
The control structure is read-only everywhere else but in main(). Almost
all changes are about how variables are referred, with one exception. Calls
to read_adjtime() from manipulate_clock() and compare_clock() are moved to
main(). This way it is possible to keep variable that tells if hwclock is
using UTC-0 be part of control structure.
Changes within #ifdef __alpha__ segments were tested by flipping the
preprocessor directivive otherway around and getting good compilaton all the
way to the point where linking on none-alpha system failed.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
8< -------
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
8< -------
@@ -1087,51 +1074,51 @@ calculate_adjustment(const double factor,
* But if the contents are clean (unchanged since read from disk), don't
* bother.
*/
-static void save_adjtime(const struct adjtime adjtime, const bool testing)
+static void save_adjtime(const struct hwclock_control *ctl, const struct adjtime *adjtime)
>
> Wraps, this line is too long.
>
8< -------
@@ -1226,16 +1211,9 @@ static void determine_clock_access_method(const bool user_requests_ISA)
* Return rc == 0 if everything went OK, rc != 0 if not.
*/
static int
-manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
- const bool set, const time_t set_time,
- const bool hctosys, const bool systohc, const bool systz,
- const struct timeval startup_time,
- const bool utc, const bool local_opt, const bool update,
- const bool testing, const bool predict, const bool get)
+manipulate_clock(const struct hwclock_control *ctl, time_t set_time,
+ const struct timeval startup_time, struct adjtime *adjtime)
>
> Should be: const time_t set_time
>
8< -------
@@ -1255,32 +1233,27 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
/* local return code */
int rc = 0;
- if (!systz && !predict) {
+ if (!ctl->systz && !ctl->predict) {
no_auth = ur->get_permissions();
if (no_auth)
return EX_NOPERM;
}
- if (!noadjfile && !(systz && (utc || local_opt))) {
- rc = read_adjtime(&adjtime);
- if (rc)
- return rc;
- } else {
- /* A little trick to avoid writing the file if we don't have to */
- adjtime.dirty = FALSE;
>
> You have to move the above to where you read_adjtime in main. The
> concept behind --systz is speed, so we don't want to read the file
> then unless they also are changing timescales from the command line.
> It is not intended to use --systz for changing timescales, so that
> shouldn't happen. More importantly, hwclock depends on *adjtime having
> default values when using --noadjfile. For example comparing the
> patched version to master using the --get function:
./hwclock-master --get --debug --noadjfile --utc | grep -E 'Calculated|2017\-'; ./hwclock-sami --get --debug --noadjfile --utc | grep -E 'Calculated|2017\-'
Calculated Hardware Clock drift is 0.000000 seconds
2017-01-06 10:11:32.294942-0500
Calculated Hardware Clock drift is 1.325654 seconds
2017-01-06 10:11:34.339980-0500
>
>
>
+ if ((ctl->set || ctl->systohc || ctl->adjust) &&
+ (adjtime->local_utc == UTC) != ctl->universal) {
+ adjtime->local_utc = ctl->universal ? UTC : LOCAL;
+ adjtime->dirty = TRUE;
}
- universal = hw_clock_is_utc(utc, local_opt, adjtime);
-
- if ((set || systohc || adjust) &&
- (adjtime.local_utc == UTC) != universal) {
- adjtime.local_utc = universal ? UTC : LOCAL;
- adjtime.dirty = TRUE;
+ if (ctl->noadjfile || (ctl->systz && (ctl->utc || ctl->local_opt))) {
+ /* A little trick to avoid writing the file if we don't have to */
+ adjtime->dirty = FALSE;
>
> So the above addition won't be here.
>
8< -------
>
> This is really nice Sami, thanks. I volunteered to work on
> refactoring hwclock long ago and have not yet found time to get to it.
> This patch_set goes a long way to that goal.
>
commit a5edff19900950a2b0b0ee95461c77ae2a38328c
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sat Jul 16 13:43:35 2016 +0100
hwclock: remove unnecessary type casts
Most of the casts did nothing, with exception of couple printouts where
format specifier is updated to match with the variable type.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
8< -------
>
> Nice, thank you.
>
commit df8c1aa40b0f08350bb0e4a6dbb14a2b6a57b42f
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sat Jul 16 13:23:28 2016 +0100
hwclock: do not hardcode date command magic string twice
Variable 'magic' already contains string 'seconds-into-epoch'.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
8< -------
>
> Nice, thank you.
>
commit 1b65c7fb412c117cef326789ebcbf9beb0d81e56
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sat Jul 16 12:57:25 2016 +0100
hwclock: remove hwclock_exit() indirection
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
8< -------
>
> Nice, thank you.
>
commit 00999f9555fbf68e4f1957c55bde5a99aa5e8484
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sat Jul 16 12:50:53 2016 +0100
hwclock: remove FLOOR macro
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 1d193a5..b517dcb 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -92,8 +92,6 @@ static int hwaudit_on;
/* The struct that holds our hardware access routines */
struct clock_ops *ur;
-#define FLOOR(arg) ((arg >= 0 ? (int) arg : ((int) arg) - 1));
-
/* Maximal clock adjustment in seconds per day.
(adjtime() glibc call has 2145 seconds limit on i386, so it is good enough for us as well,
43219 is a maximal safe value preventing exact_adjustment overflow.) */
@@ -1068,7 +1066,9 @@ calculate_adjustment(const double factor,
exact_adjustment =
((double)(systime - last_time)) * factor / (24 * 60 * 60)
+ not_adjusted;
- tdrift_p->tv_sec = FLOOR(exact_adjustment);
+ tdrift_p->tv_sec = (int) exact_adjustment;
+ if (exact_adjustment < 0)
+ tdrift_p->tv_sec--;
tdrift_p->tv_usec = (exact_adjustment -
(double)tdrift_p->tv_sec) * 1E6;
if (debug) {
>
> If I look at this change in 5 years, the intent will not be clear. If
> I see FLOOR() I'll know exactly what it is. What's wrong with having a
> floor macro? It could be a floor function, if you don't like it as a
> macro?
>
commit 6a8b954f5a7d2ab782047f15ae0da0b25c7aed8d
Author: Sami Kerola <kerolasa@iki.fi>
Date: Tue Jul 12 22:21:10 2016 +0100
lib: add timegm() portability function to lib/timeutils.c
Local timegm() is a replacement function in cases it is missing from libc
implementation. Hopefully the replacement is never, or very rarely, used.
CC: Ruediger Meier <ruediger.meier@ga-group.nl>
CC: J William Piggott <elseifthen@gmx.com>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
8< -------
>
> Nice, thank you.
>
commit c6fa71e3af966af9376c96a69f78d2547ea00cc2
Author: Sami Kerola <kerolasa@iki.fi>
Date: Sun Jul 10 20:09:55 2016 +0100
hwclock: remove UTC-0 localization hack
Use timegm(3) instead rather than re-implement same functionality with
mktime(3) combined with removal of TZ localization.
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
8< -------
>
> Nice, thank you.
>
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-07 19:37 ` J William Piggott
@ 2017-01-07 20:32 ` J William Piggott
2017-01-07 23:06 ` Sami Kerola
1 sibling, 0 replies; 28+ messages in thread
From: J William Piggott @ 2017-01-07 20:32 UTC (permalink / raw)
To: Sami Kerola, util-linux
On 01/07/2017 02:37 PM, J William Piggott wrote:
>
>
+ } else {
> + if (ctl->debug)
> printf(_("No usable clock interface found.\n"));
> + warnx(_("Cannot access the Hardware Clock via "
> + "any known method."));
>
>>
>> warnx( indentation is wrong.
>>
>
Forget this one, I looked at it wrong.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-07 19:37 ` J William Piggott
2017-01-07 20:32 ` J William Piggott
@ 2017-01-07 23:06 ` Sami Kerola
2017-01-08 9:39 ` Sami Kerola
` (3 more replies)
1 sibling, 4 replies; 28+ messages in thread
From: Sami Kerola @ 2017-01-07 23:06 UTC (permalink / raw)
To: J William Piggott; +Cc: util-linux
On Sat, 7 Jan 2017, J William Piggott wrote:
> Below is a monolithic review of the pull request using reverse quoting
> so the code is easier to view/highlight.
>
> Edit: I sent this once and it choked somewhere. So below is highly
> redacted version.
Thank you JWP so much for review. I am sure Karel is also happy to see
other than himself reading changes. After your review there is new remote
branch in my repository
git://github.com/kerolasa/lelux-utiliteetit.git hwclock-jwp-reviewed
that has in almost all changes 'Reviewed-by: J William Piggott
<elseifthen@gmx.com>'. Notice also that the latest update is not 100%
ready. We have question of --compare open, and 'hwclock: move command-line
options to control structure' has issue that is not yet corrected yet.
These are the differences so far with former 'hwclock' branch.
-- snip
diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
index 008bfba38..411ec433a 100644
--- a/sys-utils/hwclock-rtc.c
+++ b/sys-utils/hwclock-rtc.c
@@ -261,7 +261,7 @@ static int synchronize_to_clock_tick_rtc(const struct hwclock_control *ctl)
#else
rc = ioctl(rtc_fd, RTC_UIE_ON, 0);
#endif
- if (rc == 0) {
+ if (rc != -1) {
/*
* Just reading rtc_fd fails on broken hardware: no
* update interrupt comes and a bootscript with a
diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 2c92fbf71..797f51423 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -655,7 +655,8 @@ display_time(const bool hclock_valid, struct timeval hwctime)
* If anything goes wrong (and many things can), we return return code 10
* and arbitrary *time_p. Otherwise, return code is 0 and *time_p is valid.
*/
-static int interpret_date_string(const struct hwclock_control *ctl, time_t * const time_p)
+static int interpret_date_string(const struct hwclock_control *ctl,
+ time_t *const time_p)
{
FILE *date_child_fp = NULL;
char *date_command = NULL;
@@ -673,7 +674,7 @@ static int interpret_date_string(const struct hwclock_control *ctl, time_t * con
/* Quotes in date_opt would ruin the date command we construct. */
if (strchr(ctl->date_opt, '"') != NULL ||
strchr(ctl->date_opt, '`') != NULL ||
- strstr(ctl->date_opt, "$(") != NULL) {
+ strchr(ctl->date_opt, '$') != NULL) {
warnx(_
("The value of the --date option is not a valid date.\n"
"In particular, it contains illegal character(s)."));
@@ -1031,9 +1032,7 @@ calculate_adjustment(const struct hwclock_control *ctl,
exact_adjustment =
((double)(systime - last_time)) * factor / (24 * 60 * 60)
+ not_adjusted;
- tdrift_p->tv_sec = (int) exact_adjustment;
- if (exact_adjustment < 0)
- tdrift_p->tv_sec--;
+ tdrift_p->tv_sec = (int) floor(exact_adjustment);
tdrift_p->tv_usec = (exact_adjustment -
(double)tdrift_p->tv_sec) * 1E6;
if (ctl->debug) {
@@ -1052,7 +1051,8 @@ calculate_adjustment(const struct hwclock_control *ctl,
* But if the contents are clean (unchanged since read from disk), don't
* bother.
*/
-static void save_adjtime(const struct hwclock_control *ctl, const struct adjtime *adjtime)
+static void save_adjtime(const struct hwclock_control *ctl,
+ const struct adjtime *adjtime)
{
char *content; /* Stuff to write to disk file */
FILE *fp;
@@ -1181,7 +1181,7 @@ static void determine_clock_access_method(const struct hwclock_control *ctl)
* Return rc == 0 if everything went OK, rc != 0 if not.
*/
static int
-manipulate_clock(const struct hwclock_control *ctl, time_t set_time,
+manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
const struct timeval startup_time, struct adjtime *adjtime)
{
/* The time at which we read the Hardware Clock */
-- snip
There are replies to feedback below. I can only hope your email client is
helping to find them as this message is a bit long.
> commit c7a8f56920c738a90038cd6baaaec676c642b926
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sat Dec 31 21:29:27 2016 +0000
>
> hwclock: remove trailing dot from messages that include system error message
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> >
> > Nice, thank you.
> >
> > FWIW, I think this should apply to all message strings. (Apparently these
> > types of changes annoy translators :)
> >
Correct. That is why I left most of the strings alone, but the few that
ended with dot and had trailing system error output were too ugly. With
a dot they look like:
hwclock: important message.: system error
That 'trailing' dot is not at the end of the line, but before : making it
to be untolerable.
> commit 3d3444cfc24c843e9efc65a56e78f8060d438a7c
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Wed Jul 27 19:47:38 2016 +0100
>
> hwclock: make --date=argument less prone to injection
>
> This change should not improve security much. One hopes hwclock --set is
> restricted for root only. Where hwclock is allowed to run via sudo, or has
> setuid setup, there is a pretty easy privilege escalation via subshell.
>
> $ sudo ./hwclock --set --date='2000-10-20$(touch /tmp/hwclock.inject)'
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
> index 9a5cb8f..2c92fbf 100644
> --- a/sys-utils/hwclock.c
> +++ b/sys-utils/hwclock.c
> @@ -671,10 +671,12 @@ static int interpret_date_string(const struct hwclock_control *ctl, time_t * con
> }
>
> /* Quotes in date_opt would ruin the date command we construct. */
> - if (strchr(ctl->date_opt, '"') != NULL) {
> + if (strchr(ctl->date_opt, '"') != NULL ||
> + strchr(ctl->date_opt, '`') != NULL ||
> + strstr(ctl->date_opt, "$(") != NULL) {
> warnx(_
> ("The value of the --date option is not a valid date.\n"
> - "In particular, it contains quotation marks."));
> + "In particular, it contains illegal character(s)."));
> return retcode;
> }
>
> >
> > Nice, thank you. I think just $ would have been enough.
> >
Good point. Banning variables is likely a good idea to avoid variable
contents smuggling explosive content. Changed in proposed way.
> commit fec97d35eb42601a347d5cac7ddd5b1d422edbbe
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Tue Jul 26 14:34:30 2016 +0100
>
> hwclock: fix rtc atexit registration
>
> Commit 27f9db17bd57b85947445c03e2cd9dda36ca377f missed a minus sign from
> comparison.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
> index 9a67027..7e1a945 100644
> --- a/sys-utils/hwclock-rtc.c
> +++ b/sys-utils/hwclock-rtc.c
> @@ -138,7 +138,7 @@ static int open_rtc(const struct hwclock_control *ctl)
> if (rtc_dev_fd < 0)
> rtc_dev_name = *fls; /* default for error messages */
> }
> - if (rtc_dev_fd != 1)
> + if (rtc_dev_fd != -1)
> atexit(close_rtc);
> return rtc_dev_fd;
> }
>
> >
> > Nice catch, thank you.
> >
>
>
> commit 6f62eca023c0ec187a369cc668a962c696fd1107
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Tue Jul 26 08:33:32 2016 +0100
>
> hwclock: clarify cmos inb and outb preprocessor directives
>
> The cmos only works when architecture is i386, x86_64, or alpha.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> >
> > Nice, thank you.
> >
>
>
> commit d9ccb70cafaac62fd6c8294fbba3b3ab385d3537
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Tue Jul 26 10:52:15 2016 +0100
>
> hwclock: try RTCGET and RTCSET only when normal rtc fails
>
> The RTCGET and RTCSET are in use for sparcs with sbus, so try them as
> fallback rather than always.
>
> Reference: https://github.com/torvalds/linux/blob/master/fs/compat_ioctl.c#L967-L974
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
> index fc1dfc3..9a67027 100644
> --- a/sys-utils/hwclock-rtc.c
> +++ b/sys-utils/hwclock-rtc.c
> @@ -158,28 +158,31 @@ static int do_rtc_read_ioctl(int rtc_fd, struct tm *tm)
> {
> int rc = -1;
> char *ioctlname;
> -
> #ifdef __sparc__
> /* some but not all sparcs use a different ioctl and struct */
> struct sparc_rtc_time stm;
> +#endif
>
> 8< -------
>
> >
> > Can't this be in the sparc ifdef just after this?
> >
> > Otherwise looks OK, thank you.
> >
That would mix declaration and code. Kept as is.
> commit 08e959db08840871979b00036d85aa6941f9fa76
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Tue Jul 26 08:54:00 2016 +0100
>
> hwclock: stream line synchronize_to_clock_tick_rtc()
>
> Flip if clauses to hit common case first. This should be easier and quicker
> to read and run.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
> index f54e263..fc1dfc3 100644
> --- a/sys-utils/hwclock-rtc.c
> +++ b/sys-utils/hwclock-rtc.c
>
> 8< -------
>
> @@ -287,26 +276,34 @@ static int synchronize_to_clock_tick_rtc(const struct hwclock_control *ctl)
> tv.tv_sec = 10;
> tv.tv_usec = 0;
> rc = select(rtc_fd + 1, &rfds, NULL, NULL, &tv);
> - ret = 1;
> - if (rc == -1)
> - warn(_("select() to %s to wait for clock tick failed"),
> - rtc_dev_name);
> + if (0 < rc)
> + ret = 0;
> else if (rc == 0) {
> if (ctl->debug)
> printf(_("select() to %s to wait for clock tick timed out"),
> rtc_dev_name);
> } else
> - ret = 0;
> + warn(_("select() to %s to wait for clock tick failed"),
> + rtc_dev_name);
> /* Turn off update interrupts */
> rc = ioctl(rtc_fd, RTC_UIE_OFF, 0);
> if (rc == -1)
> warn(_("ioctl() to %s to turn off update interrupts failed"),
> rtc_dev_name);
>
> >
> >
> > The /* Turn off update interrupts */ code was the success block. You
> > didn't move it up to your new success block at 'if (0 < rc)'. The
> > interrupts will never be turned off with this patch.
> >
> >
>
> 8< -------
I looked this change, and especially result of the change, again as I
could not anymore tell what that diff does. This did indeed need change;
ioctl returns not -1 when it is successful, so I am checking that rather
than if it returns 0. Content of the 'success' block is left asis.
Notice that I did not include your review signature to this change because
I it does not sound you would be too happy about that before another look.
> commit bb08719b0160d560defe091e60687d5d1240e88c
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sun Jul 24 22:33:01 2016 +0100
>
> hwclock: improve coding style
>
> Make string constants to be symbolical declarations. Use longer variable
> name for rtc and cmos function pointer values. Exclude code that is
> architecture specific with preprocessor directives. And remove message
> duplication.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> >
> > Nice, thank you.
> >
>
>
> commit 80b6f08f52e21205f41de0a51a26abd4034cf0cf
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sat Jul 23 12:20:03 2016 +0100
>
> docs: deprecate hwclock --compare option
>
> Earlier commit clarified arithmetics that implement hwclock --compare
> option, and that functionality feels like rubbish in - rubbish out. This
> surely cannot be correct way to find frequency offset and/or tick. Even if
> in principle doing calculations this way makes sense, the measurements are
> prone to get skew due scheduling and other environment effects. Therefore
> mark this option deprecated.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> diff --git a/sys-utils/hwclock.8.in b/sys-utils/hwclock.8.in
> index de68e43..58b5158 100644
> --- a/sys-utils/hwclock.8.in
> +++ b/sys-utils/hwclock.8.in
> @@ -45,6 +45,9 @@ discussion below, under
> Periodically compare the Hardware Clock to the System Time and output
> the difference every 10 seconds. This will also print the frequency
> offset and tick.
> +.IP
> +The compare output is not a good measurement of offset or tick.
> +This functionality is deprecated, and will be removed in future.
> .
> .TP
> .B \-\-getepoch
> diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
> index 774701f..9a5cb8f 100644
> --- a/sys-utils/hwclock.c
> +++ b/sys-utils/hwclock.c
> @@ -1385,6 +1385,9 @@ static inline time_t get_times(const struct hwclock_control *ctl,
> /*
> * Compare the system and CMOS time and output the drift
> * in 10 second intervals.
> + *
> + * FIXME: remove this function, unused functions, related option parsing,
> + * and manual entry when ever after 2019-07-01.
> */
> static int compare_clock(const struct hwclock_control *ctl)
> {
>
> >
> >
> > I tried to convince Karel of this long ago. This code has been broken
> > from day one, so nobody can be using it. IMO, it should be removed now.
> >
> >
I agree. --compare is broken. I think there are couple options.
1) remove --compare code, and mention in manual page it's gone
2) keep the code, but deprecate it
3) keep the code, and fix it (but how???)
4) keep the broken code and whistle happy days theme song
5) something else
I am 60-40 leaning towards recommending removal. If we don't remove rest
of my 40 goes to deprecate. Karel, I think we need maintainer advice.
> commit 2c2bfd4828ced9f3ca1f0288d6447a0bd8f62bba
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Fri Jul 22 22:45:35 2016 +0100
>
> hwclock: use integer arithmetics in compare_clock()
>
> Almost all floating point arithmetics are now converted to interger
> arithmetics in compare_clock(). In same go variables names are renamed to
> be hopefully a little more easier to understand. The second time
> substraction got new comment because it took a while to understand out what
> the code tried to do.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
>
> 8< -------
>
> >
> > Not reviewing. It's pointless as --compare is still broken.
> >
> >
ACK. See my reply above.
> commit 1ab5409350480e7eb1b4c063eae75f8bb35d6bfe
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sun Jul 17 22:04:32 2016 +0100
>
> hwclock: remove division by zero [asan]
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> 8< -------
>
> >
> > Not reviewing.
> >
> >
>
>
>
> commit 8a0279382b4be44a173a8e9419a7fd211fcb1443
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sun Jul 17 17:10:48 2016 +0100
>
> hwclock: add debugging to open_rtc()
>
> Earlier when open_rtc() returned -1 the char *rtc_dev_name end up having
> NULL that made it unsuitable to be used in error message. Now one can debug
> what paths the open_rtc() tries to use when one has to debug why 'cannot
> open rtc device' happen.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> 8< -------
>
>
> >
> > Nice, thank you.
> >
> >
>
>
> commit 4c4965c0e98da9e6b4e64de4c1dff7d41eb81018
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sun Jul 17 16:35:40 2016 +0100
>
> hwclock: remove magic constants from interpret_date_string()
>
> The constants function returned were not used. In same go clean up
> execution flow a little bit.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> 8< -------
> >
> > Nice, thank you.
> >
>
>
> commit 1d3829c027cb5e09e698b803a42a523d49f7815f
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sun Jul 17 16:14:16 2016 +0100
>
> hwclock: use symbolic magic values passed in between functions
>
> The manipulate_clock() is seeing return value from
> busywait_for_rtc_clock_tick().
>
> And the get_permissions_cmos() can see i386_iopl() return value.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> 8< -------
>
> >
> > Nice, thank you.
> >
> >
>
>
> commit 017de7d8231aaaf8cd57fa9b0887e949c9c4c683
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sun Jul 17 13:25:40 2016 +0100
>
> hwclock: initialize struct adjtime members
>
> Avoid any change of using uninitialized values. By looks of it the earlier
> code did take care of that, but was less obvious about the fact.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> >
> > s/change/chance/
> >
> > Above is the important change, but as long as you're fixing that:
> >
> > s/By looks of it/It looks like/
> > s/but/but it/
>
>
> 8< -------
>
> >
> > Otherwise nice, thank you.
> >
Updated.
> commit c4e533646fef70dbd8ae2bbc2e3becb1954b5353
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sun Jul 17 12:49:12 2016 +0100
>
> hwclock: alloate date_resp parsing buffer in interpret_date_string()
>
> This makes overflowing the variable in question impossible.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> 8< -------
>
> >
> > Nice, thank you.
> >
>
>
> commit 573a993964bd695b68c62a9eb75fdd312ad3f836
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sun Jul 17 12:12:52 2016 +0100
>
> hwclock: simplify save_adjtime() execution flow
>
> Return early to avoid excessive nesting. In same go remove any change of
> overflow by using appropriate allocation. And update variable names to be
> easier to understand.
>
> >
> > s/change/chance/
> >
>
> 8< -------
>
> >
> > Otherwise looks OK, thank you.
> >
Updated.
> commit 285ac4c3899d0876f53591962ac5119607d71474
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sun Jul 17 11:30:40 2016 +0100
>
> hwclock: remove dead code and other minor fixes
>
> Use #ifdef rather than #if to avoid undefined preprocessor identifier
> warning.
>
> Remove dead code. The #if 0 ensured the code has not been used for long
> time, which is good because the linux/mc146818rtc.h is not been part of
> user-api for long time.
>
> Value of the adjtime_p->last_calib_time is checked if it has value of zero,
> so testing none-zero bit later is necessarily true, and therefore does not
> need to be checked.
>
> And at the and remove unnecessary boolean variable.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> 8< -------
>
> >
> > Looks OK, thank you.
> >
>
>
>
> commit 509ac719dbe1bcc4a761f694497493525da39e25
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sun Jul 17 09:09:33 2016 +0100
>
> docs: add instructions how to avoid alpha epoc Y2020 bug
>
> Recommend setting epoch using command line to for Alpha to avoid
> autodetection that is doomed to afail after 2020.
>
> Reference: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/alpha/kernel/rtc.c?id=85d0b3a573d8b711ee0c96199ac24a0f3283ed68
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> diff --git a/sys-utils/hwclock-cmos.c b/sys-utils/hwclock-cmos.c
> index 9d9a42a..cf26064 100644
> --- a/sys-utils/hwclock-cmos.c
> +++ b/sys-utils/hwclock-cmos.c
> @@ -203,15 +203,16 @@ void set_cmos_epoch(const struct hwclock_control *ctl)
> #endif
>
> /*
> - * The kernel source today says: read the year.
> + * The automatic epoc determination in kernel
> + * linux/arch/alpha/kernel/rtc.c
> *
> * If it is in 0-19 then the epoch is 2000.
> * If it is in 20-47 then the epoch is 1980.
> * If it is in 48-69 then the epoch is 1952.
> * If it is in 70-99 then the epoch is 1928.
> *
> - * Otherwise the epoch is 1900.
> - * TODO: Clearly, this must be changed before 2019.
> + * Alpha users should explicitly define epoc using rtc kernel module
> + * option rtc epoc=<year>
> */
> /*
> * See whether we are dealing with SRM or MILO, as they have
> diff --git a/sys-utils/hwclock.8.in b/sys-utils/hwclock.8.in
> index 9534c19..de68e43 100644
> --- a/sys-utils/hwclock.8.in
> +++ b/sys-utils/hwclock.8.in
> @@ -466,6 +466,14 @@ For example, on a Digital Unix machine:
> .IP "" 4
> .B hwclock\ \-\-setepoch\ \-\-epoch=1952
> .RE
> +.IP
> +Beginning January 1, 2020 automatic epoc determination in kernel for systems
> +using 1900 will fail. In these cases it is recommended to specify epoc
> +explicitly with kernel module option.
> +.RS
> +.IP "" 4
> +.B options rtc epoch=1900
> +.RE
> .
> .TP
> .B \-\-funky\-toy
>
>
> >
> > I don't like this one because:
> >
> > Despite Richard's "doomed to fail" comment, I don't think util-linux
> > should document that as a fact. The heuristics could be updated. If the
> > kernel is still supporting DEC in 2019 it seems like they should
> > update them. It would be more work to remove the code than to adjust
> > the numbers.
> >
> > Also the patch has some errors:
> >
> > * The possible failure applies to more than 'systems using 1900'.
> >
> > * The Alpha rtc driver cannot be built as a module; I believe
> > epoch={year} is a kernel command line argument only.
> >
> > * The heuristics in drivers/char/rtc.c are slightly different than
> > what the patch references in linux/arch/alpha/kernel/rtc.c. I don't
> > know which one wins. They both differ from how they are described in
> > the hwclock-cmos.c comments, from which this patch deletes the 1900
> > possibility.
> >
> > The Alpha auto epoch detect has nothing to do with hwclock --setepoch
> > --epoch {year}. That is all done via rtc and can only set it manually.
> > Auto detecting the epoch only affects using --directisa on Alpha
> > machines. Direct access requires hwclock to handle the epoch offset
> > internally, because we're bypassing the kernel. So hwclock-cmos.c has
> > to figure out what to use. It tries heuristics and, ironically, it
> > tries to use the kernel's auto detect via the rtc driver. Why use
> > direct access if you have a working rtc interface!
> >
> > It is undocumented, but when using --directisa on alpha --epoch {year}
> > can be used to bypass auto detect. So --epoch {year} would be used for
> > all functions. The man-page says it can only be used with --setepoch.
> > I have no way to test this, so until now I have not documented it.
> >
I left the change as-is for now, making the branch in it's current state
not to be ready. How about if I read carefully what you said above,
rethink what I thought earlier, and reply later.
> commit 6deb1326df19a87a97a5e77550f87c31a0367c92
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sat Jul 16 22:15:54 2016 +0100
>
> hwclock: move error messages to determine_clock_access_method()
>
> This makes main() a little bit shorter.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> 8< -------
>
> --- a/sys-utils/hwclock.c
> +++ b/sys-utils/hwclock.c
> @@ -1189,17 +1189,24 @@ static void determine_clock_access_method(const struct hwclock_control *ctl)
>
> if (ctl->directisa)
> ur = probe_for_cmos_clock();
> -
> #ifdef __linux__
> if (!ur)
> ur = probe_for_rtc_clock(ctl);
> #endif
> + if (ur) {
> + if (ctl->debug)
> + puts(ur->interface_name);
>
> - if (ctl->debug) {
> - if (ur)
> - puts(_(ur->interface_name));
> - else
> + } else {
> + if (ctl->debug)
> printf(_("No usable clock interface found.\n"));
> + warnx(_("Cannot access the Hardware Clock via "
> + "any known method."));
>
> >
> > warnx( indentation is wrong.
> >
>
> 8< -------
>
> >
> > Otherwise looks OK, thank you.
> >
>
>
> commit bf8f8287b095f9b1401253ecea46bab5ac96d811
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sat Jul 16 20:18:19 2016 +0100
>
> hwclock: clarify set_cmos_epoch() code
>
> Variable set_epoc is unnecessary, and removal of it makes it obvious what is
> happening in this function.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> 8< -------
>
> >
> > Looks OK, thank you.
> >
>
>
> commit 59aff057349335cd089c34bb0eaa972cf60ff96a
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sat Jul 16 16:45:07 2016 +0100
>
> hwclock: move command-line options to control structure
>
> The control structure is read-only everywhere else but in main(). Almost
> all changes are about how variables are referred, with one exception. Calls
> to read_adjtime() from manipulate_clock() and compare_clock() are moved to
> main(). This way it is possible to keep variable that tells if hwclock is
> using UTC-0 be part of control structure.
>
> Changes within #ifdef __alpha__ segments were tested by flipping the
> preprocessor directivive otherway around and getting good compilaton all the
> way to the point where linking on none-alpha system failed.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> 8< -------
>
> --- a/sys-utils/hwclock.c
> +++ b/sys-utils/hwclock.c
>
> 8< -------
>
> @@ -1087,51 +1074,51 @@ calculate_adjustment(const double factor,
> * But if the contents are clean (unchanged since read from disk), don't
> * bother.
> */
> -static void save_adjtime(const struct adjtime adjtime, const bool testing)
> +static void save_adjtime(const struct hwclock_control *ctl, const struct adjtime *adjtime)
>
>
> >
> > Wraps, this line is too long.
> >
Wrapping is now done earlier here and where interpret_date_string()
declared.
> 8< -------
>
> @@ -1226,16 +1211,9 @@ static void determine_clock_access_method(const bool user_requests_ISA)
> * Return rc == 0 if everything went OK, rc != 0 if not.
> */
> static int
> -manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
> - const bool set, const time_t set_time,
> - const bool hctosys, const bool systohc, const bool systz,
> - const struct timeval startup_time,
> - const bool utc, const bool local_opt, const bool update,
> - const bool testing, const bool predict, const bool get)
> +manipulate_clock(const struct hwclock_control *ctl, time_t set_time,
> + const struct timeval startup_time, struct adjtime *adjtime)
>
>
> >
> > Should be: const time_t set_time
> >
Changd.
> 8< -------
>
> @@ -1255,32 +1233,27 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
> /* local return code */
> int rc = 0;
>
> - if (!systz && !predict) {
> + if (!ctl->systz && !ctl->predict) {
> no_auth = ur->get_permissions();
> if (no_auth)
> return EX_NOPERM;
> }
>
> - if (!noadjfile && !(systz && (utc || local_opt))) {
> - rc = read_adjtime(&adjtime);
> - if (rc)
> - return rc;
> - } else {
> - /* A little trick to avoid writing the file if we don't have to */
> - adjtime.dirty = FALSE;
>
> >
> > You have to move the above to where you read_adjtime in main. The
> > concept behind --systz is speed, so we don't want to read the file
> > then unless they also are changing timescales from the command line.
> > It is not intended to use --systz for changing timescales, so that
> > shouldn't happen. More importantly, hwclock depends on *adjtime having
> > default values when using --noadjfile. For example comparing the
> > patched version to master using the --get function:
>
> ./hwclock-master --get --debug --noadjfile --utc | grep -E 'Calculated|2017\-'; ./hwclock-sami --get --debug --noadjfile --utc | grep -E 'Calculated|2017\-'
> Calculated Hardware Clock drift is 0.000000 seconds
> 2017-01-06 10:11:32.294942-0500
> Calculated Hardware Clock drift is 1.325654 seconds
> 2017-01-06 10:11:34.339980-0500
>
> >
> >
> >
>
> + if ((ctl->set || ctl->systohc || ctl->adjust) &&
> + (adjtime->local_utc == UTC) != ctl->universal) {
> + adjtime->local_utc = ctl->universal ? UTC : LOCAL;
> + adjtime->dirty = TRUE;
> }
>
> - universal = hw_clock_is_utc(utc, local_opt, adjtime);
> -
> - if ((set || systohc || adjust) &&
> - (adjtime.local_utc == UTC) != universal) {
> - adjtime.local_utc = universal ? UTC : LOCAL;
> - adjtime.dirty = TRUE;
> + if (ctl->noadjfile || (ctl->systz && (ctl->utc || ctl->local_opt))) {
> + /* A little trick to avoid writing the file if we don't have to */
> + adjtime->dirty = FALSE;
>
> >
> > So the above addition won't be here.
> >
This issue is not corrected yet. It's getting a bit too late in my
timezone to do change that requires concentration. I will send notify to
email list when necessary update is done.
> 8< -------
>
> >
> > This is really nice Sami, thanks. I volunteered to work on
> > refactoring hwclock long ago and have not yet found time to get to it.
> > This patch_set goes a long way to that goal.
> >
>
>
> commit a5edff19900950a2b0b0ee95461c77ae2a38328c
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sat Jul 16 13:43:35 2016 +0100
>
> hwclock: remove unnecessary type casts
>
> Most of the casts did nothing, with exception of couple printouts where
> format specifier is updated to match with the variable type.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> 8< -------
>
> >
> > Nice, thank you.
> >
>
>
> commit df8c1aa40b0f08350bb0e4a6dbb14a2b6a57b42f
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sat Jul 16 13:23:28 2016 +0100
>
> hwclock: do not hardcode date command magic string twice
>
> Variable 'magic' already contains string 'seconds-into-epoch'.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> 8< -------
>
> >
> > Nice, thank you.
> >
>
>
> commit 1b65c7fb412c117cef326789ebcbf9beb0d81e56
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sat Jul 16 12:57:25 2016 +0100
>
> hwclock: remove hwclock_exit() indirection
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> 8< -------
>
> >
> > Nice, thank you.
> >
>
>
> commit 00999f9555fbf68e4f1957c55bde5a99aa5e8484
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sat Jul 16 12:50:53 2016 +0100
>
> hwclock: remove FLOOR macro
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
> index 1d193a5..b517dcb 100644
> --- a/sys-utils/hwclock.c
> +++ b/sys-utils/hwclock.c
> @@ -92,8 +92,6 @@ static int hwaudit_on;
> /* The struct that holds our hardware access routines */
> struct clock_ops *ur;
>
> -#define FLOOR(arg) ((arg >= 0 ? (int) arg : ((int) arg) - 1));
> -
> /* Maximal clock adjustment in seconds per day.
> (adjtime() glibc call has 2145 seconds limit on i386, so it is good enough for us as well,
> 43219 is a maximal safe value preventing exact_adjustment overflow.) */
> @@ -1068,7 +1066,9 @@ calculate_adjustment(const double factor,
> exact_adjustment =
> ((double)(systime - last_time)) * factor / (24 * 60 * 60)
> + not_adjusted;
> - tdrift_p->tv_sec = FLOOR(exact_adjustment);
> + tdrift_p->tv_sec = (int) exact_adjustment;
> + if (exact_adjustment < 0)
> + tdrift_p->tv_sec--;
> tdrift_p->tv_usec = (exact_adjustment -
> (double)tdrift_p->tv_sec) * 1E6;
> if (debug) {
>
>
> >
> > If I look at this change in 5 years, the intent will not be clear. If
> > I see FLOOR() I'll know exactly what it is. What's wrong with having a
> > floor macro? It could be a floor function, if you don't like it as a
> > macro?
> >
I should have read Makefile.am earlier to notice that floor(3) can be used
here without adding libm linking, it is already linked to hwclock! After
noticing that FLOOR() to floor() change was done.
> commit 6a8b954f5a7d2ab782047f15ae0da0b25c7aed8d
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Tue Jul 12 22:21:10 2016 +0100
>
> lib: add timegm() portability function to lib/timeutils.c
>
> Local timegm() is a replacement function in cases it is missing from libc
> implementation. Hopefully the replacement is never, or very rarely, used.
>
> CC: Ruediger Meier <ruediger.meier@ga-group.nl>
> CC: J William Piggott <elseifthen@gmx.com>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> 8< -------
>
> >
> > Nice, thank you.
> >
>
>
> commit c6fa71e3af966af9376c96a69f78d2547ea00cc2
> Author: Sami Kerola <kerolasa@iki.fi>
> Date: Sun Jul 10 20:09:55 2016 +0100
>
> hwclock: remove UTC-0 localization hack
>
> Use timegm(3) instead rather than re-implement same functionality with
> mktime(3) combined with removal of TZ localization.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>
> 8< -------
>
> >
> > Nice, thank you.
> >
>
>
>
--
Sami Kerola
http://www.iki.fi/kerolasa/
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-07 23:06 ` Sami Kerola
@ 2017-01-08 9:39 ` Sami Kerola
2017-01-08 21:21 ` J William Piggott
2017-01-08 10:09 ` Sami Kerola
` (2 subsequent siblings)
3 siblings, 1 reply; 28+ messages in thread
From: Sami Kerola @ 2017-01-08 9:39 UTC (permalink / raw)
To: J William Piggott; +Cc: util-linux
On 7 January 2017 at 23:06, Sami Kerola <kerolasa@iki.fi> wrote:
> On Sat, 7 Jan 2017, J William Piggott wrote:
> git://github.com/kerolasa/lelux-utiliteetit.git hwclock-jwp-reviewed
>
> that has in almost all changes 'Reviewed-by: J William Piggott
> <elseifthen@gmx.com>'. Notice also that the latest update is not 100%
> ready. We have question of --compare open, and 'hwclock: move command-line
> options to control structure' has issue that is not yet corrected yet.
hwclock-jwp-reviewed got update:
-- snip
diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index b41acd450..c3bcc2fa0 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -1245,11 +1245,6 @@ manipulate_clock(const struct hwclock_control
*ctl, const time_t set_time,
adjtime->dirty = TRUE;
}
- if (ctl->noadjfile || (ctl->systz && (ctl->utc || ctl->local_opt))) {
- /* A little trick to avoid writing the file if we don't have to */
- adjtime->dirty = FALSE;
- }
-
if (ctl->show || ctl->get || ctl->adjust || ctl->hctosys
|| (!ctl->noadjfile && !ctl->systz && !ctl->predict)) {
/* data from HW-clock are required */
@@ -1558,7 +1553,7 @@ int main(int argc, char **argv)
{
struct hwclock_control ctl = { 0 };
struct timeval startup_time;
- struct adjtime adjtime;
+ struct adjtime adjtime = { 0 };
/*
* The time we started up, in seconds into the epoch, including
* fractions.
@@ -1839,8 +1834,12 @@ int main(int argc, char **argv)
}
}
- if ((rc = read_adjtime(&ctl, &adjtime)) != 0)
- hwclock_exit(&ctl, rc);
+ if (!ctl.noadjfile && !(ctl.systz && (ctl.utc || ctl.local_opt))) {
+ if ((rc = read_adjtime(&ctl, &adjtime)) != 0)
+ hwclock_exit(&ctl, rc);
+ } else
+ /* avoid writing adjtime file if we don't have to */
+ adjtime.dirty = FALSE;
ctl.universal = hw_clock_is_utc(&ctl, adjtime);
if (ctl.compare) {
if (compare_clock(&ctl))
-- snip
that should fix:
>> commit 59aff057349335cd089c34bb0eaa972cf60ff96a
>> Author: Sami Kerola <kerolasa@iki.fi>
>> Date: Sat Jul 16 16:45:07 2016 +0100
>>
>> hwclock: move command-line options to control structure
>>
>> The control structure is read-only everywhere else but in main(). Almost
>> all changes are about how variables are referred, with one exception. Calls
>> to read_adjtime() from manipulate_clock() and compare_clock() are moved to
>> main(). This way it is possible to keep variable that tells if hwclock is
>> using UTC-0 be part of control structure.
>>
>> Changes within #ifdef __alpha__ segments were tested by flipping the
>> preprocessor directivive otherway around and getting good compilaton all the
>> way to the point where linking on none-alpha system failed.
>>
>> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>>
>> 8< -------
>>
>> @@ -1255,32 +1233,27 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
>> /* local return code */
>> int rc = 0;
>>
>> - if (!systz && !predict) {
>> + if (!ctl->systz && !ctl->predict) {
>> no_auth = ur->get_permissions();
>> if (no_auth)
>> return EX_NOPERM;
>> }
>>
>> - if (!noadjfile && !(systz && (utc || local_opt))) {
>> - rc = read_adjtime(&adjtime);
>> - if (rc)
>> - return rc;
>> - } else {
>> - /* A little trick to avoid writing the file if we don't have to */
>> - adjtime.dirty = FALSE;
>>
>> >
>> > You have to move the above to where you read_adjtime in main. The
>> > concept behind --systz is speed, so we don't want to read the file
>> > then unless they also are changing timescales from the command line.
>> > It is not intended to use --systz for changing timescales, so that
>> > shouldn't happen. More importantly, hwclock depends on *adjtime having
>> > default values when using --noadjfile. For example comparing the
>> > patched version to master using the --get function:
>>
>> ./hwclock-master --get --debug --noadjfile --utc | grep -E 'Calculated|2017\-'; ./hwclock-sami --get --debug --noadjfile --utc | grep -E 'Calculated|2017\-'
>> Calculated Hardware Clock drift is 0.000000 seconds
>> 2017-01-06 10:11:32.294942-0500
>> Calculated Hardware Clock drift is 1.325654 seconds
>> 2017-01-06 10:11:34.339980-0500
>>
>> >
>> >
>> >
>>
>> + if ((ctl->set || ctl->systohc || ctl->adjust) &&
>> + (adjtime->local_utc == UTC) != ctl->universal) {
>> + adjtime->local_utc = ctl->universal ? UTC : LOCAL;
>> + adjtime->dirty = TRUE;
>> }
>>
>> - universal = hw_clock_is_utc(utc, local_opt, adjtime);
>> -
>> - if ((set || systohc || adjust) &&
>> - (adjtime.local_utc == UTC) != universal) {
>> - adjtime.local_utc = universal ? UTC : LOCAL;
>> - adjtime.dirty = TRUE;
>> + if (ctl->noadjfile || (ctl->systz && (ctl->utc || ctl->local_opt))) {
>> + /* A little trick to avoid writing the file if we don't have to */
>> + adjtime->dirty = FALSE;
>>
>> >
>> > So the above addition won't be here.
>> >
>
> This issue is not corrected yet. It's getting a bit too late in my
> timezone to do change that requires concentration. I will send notify to
> email list when necessary update is done.
--
Sami Kerola
http://www.iki.fi/kerolasa/
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-07 23:06 ` Sami Kerola
2017-01-08 9:39 ` Sami Kerola
@ 2017-01-08 10:09 ` Sami Kerola
2017-01-08 21:21 ` J William Piggott
2017-01-08 21:21 ` J William Piggott
2017-01-09 11:32 ` Karel Zak
3 siblings, 1 reply; 28+ messages in thread
From: Sami Kerola @ 2017-01-08 10:09 UTC (permalink / raw)
To: J William Piggott; +Cc: util-linux
On 7 January 2017 at 23:06, Sami Kerola <kerolasa@iki.fi> wrote:
> On Sat, 7 Jan 2017, J William Piggott wrote:
>> commit 509ac719dbe1bcc4a761f694497493525da39e25
>> Author: Sami Kerola <kerolasa@iki.fi>
>> Date: Sun Jul 17 09:09:33 2016 +0100
>>
>> docs: add instructions how to avoid alpha epoc Y2020 bug
>>
>> Recommend setting epoch using command line to for Alpha to avoid
>> autodetection that is doomed to afail after 2020.
>>
>> Reference: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/alpha/kernel/rtc.c?id=85d0b3a573d8b711ee0c96199ac24a0f3283ed68
>> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>>
>> diff --git a/sys-utils/hwclock-cmos.c b/sys-utils/hwclock-cmos.c
>> index 9d9a42a..cf26064 100644
>> --- a/sys-utils/hwclock-cmos.c
>> +++ b/sys-utils/hwclock-cmos.c
>> @@ -203,15 +203,16 @@ void set_cmos_epoch(const struct hwclock_control *ctl)
>> #endif
>>
>> /*
>> - * The kernel source today says: read the year.
>> + * The automatic epoc determination in kernel
>> + * linux/arch/alpha/kernel/rtc.c
>> *
>> * If it is in 0-19 then the epoch is 2000.
>> * If it is in 20-47 then the epoch is 1980.
>> * If it is in 48-69 then the epoch is 1952.
>> * If it is in 70-99 then the epoch is 1928.
>> *
>> - * Otherwise the epoch is 1900.
>> - * TODO: Clearly, this must be changed before 2019.
>> + * Alpha users should explicitly define epoc using rtc kernel module
>> + * option rtc epoc=<year>
>> */
>> /*
>> * See whether we are dealing with SRM or MILO, as they have
>> diff --git a/sys-utils/hwclock.8.in b/sys-utils/hwclock.8.in
>> index 9534c19..de68e43 100644
>> --- a/sys-utils/hwclock.8.in
>> +++ b/sys-utils/hwclock.8.in
>> @@ -466,6 +466,14 @@ For example, on a Digital Unix machine:
>> .IP "" 4
>> .B hwclock\ \-\-setepoch\ \-\-epoch=1952
>> .RE
>> +.IP
>> +Beginning January 1, 2020 automatic epoc determination in kernel for systems
>> +using 1900 will fail. In these cases it is recommended to specify epoc
>> +explicitly with kernel module option.
>> +.RS
>> +.IP "" 4
>> +.B options rtc epoch=1900
>> +.RE
>> .
>> .TP
>> .B \-\-funky\-toy
>>
>>
>> >
>> > I don't like this one because:
>> >
>> > Despite Richard's "doomed to fail" comment, I don't think util-linux
>> > should document that as a fact. The heuristics could be updated. If the
>> > kernel is still supporting DEC in 2019 it seems like they should
>> > update them. It would be more work to remove the code than to adjust
>> > the numbers.
>> >
>> > Also the patch has some errors:
>> >
>> > * The possible failure applies to more than 'systems using 1900'.
>> >
>> > * The Alpha rtc driver cannot be built as a module; I believe
>> > epoch={year} is a kernel command line argument only.
>> >
>> > * The heuristics in drivers/char/rtc.c are slightly different than
>> > what the patch references in linux/arch/alpha/kernel/rtc.c. I don't
>> > know which one wins. They both differ from how they are described in
>> > the hwclock-cmos.c comments, from which this patch deletes the 1900
>> > possibility.
>> >
>> > The Alpha auto epoch detect has nothing to do with hwclock --setepoch
>> > --epoch {year}. That is all done via rtc and can only set it manually.
>> > Auto detecting the epoch only affects using --directisa on Alpha
>> > machines. Direct access requires hwclock to handle the epoch offset
>> > internally, because we're bypassing the kernel. So hwclock-cmos.c has
>> > to figure out what to use. It tries heuristics and, ironically, it
>> > tries to use the kernel's auto detect via the rtc driver. Why use
>> > direct access if you have a working rtc interface!
>> >
>> > It is undocumented, but when using --directisa on alpha --epoch {year}
>> > can be used to bypass auto detect. So --epoch {year} would be used for
>> > all functions. The man-page says it can only be used with --setepoch.
>> > I have no way to test this, so until now I have not documented it.
>> >
>
> I left the change as-is for now, making the branch in it's current state
> not to be ready. How about if I read carefully what you said above,
> rethink what I thought earlier, and reply later.
This change is dropped.
What comes to other comments nice to hear 'doomed to fail' is not quite
as bad situation as it was made to sound. Maybe it would be appropriate to
deal alpha 2020 issue separately, with kernel upstream alpha maintainers.
Right now my only recommendation is to clear out the situation this year
(2017). After all 2020 is just round corner. We shall see whether clearning
out means code and docs changes to kernel, hwclock, or both.
--
Sami Kerola
http://www.iki.fi/kerolasa/
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-07 23:06 ` Sami Kerola
2017-01-08 9:39 ` Sami Kerola
2017-01-08 10:09 ` Sami Kerola
@ 2017-01-08 21:21 ` J William Piggott
2017-01-11 21:44 ` Sami Kerola
2017-01-09 11:32 ` Karel Zak
3 siblings, 1 reply; 28+ messages in thread
From: J William Piggott @ 2017-01-08 21:21 UTC (permalink / raw)
To: Sami Kerola; +Cc: util-linux
On 01/07/2017 06:06 PM, Sami Kerola wrote:
> On Sat, 7 Jan 2017, J William Piggott wrote:
>
8< ------
>
> These are the differences so far with former 'hwclock' branch.
>
> -- snip
> diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
> index 008bfba38..411ec433a 100644
> --- a/sys-utils/hwclock-rtc.c
> +++ b/sys-utils/hwclock-rtc.c
> @@ -261,7 +261,7 @@ static int synchronize_to_clock_tick_rtc(const struct hwclock_control *ctl)
> #else
> rc = ioctl(rtc_fd, RTC_UIE_ON, 0);
> #endif
> - if (rc == 0) {
> + if (rc != -1) {
The former worked for a long time, but this is better I think.
Nice catch, Sami.
> /*
> * Just reading rtc_fd fails on broken hardware: no
> * update interrupt comes and a bootscript with a
> diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
> index 2c92fbf71..797f51423 100644
> --- a/sys-utils/hwclock.c
> +++ b/sys-utils/hwclock.c
> @@ -655,7 +655,8 @@ display_time(const bool hclock_valid, struct timeval hwctime)
> * If anything goes wrong (and many things can), we return return code 10
> * and arbitrary *time_p. Otherwise, return code is 0 and *time_p is valid.
> */
> -static int interpret_date_string(const struct hwclock_control *ctl, time_t * const time_p)
> +static int interpret_date_string(const struct hwclock_control *ctl,
> + time_t *const time_p)
Sorry for nitpicking. I know you didn't write this, but as long as
you're making a change here, shouldn't it be: const time_t *time_p?
> {
> FILE *date_child_fp = NULL;
> char *date_command = NULL;
> @@ -673,7 +674,7 @@ static int interpret_date_string(const struct hwclock_control *ctl, time_t * con
> /* Quotes in date_opt would ruin the date command we construct. */
> if (strchr(ctl->date_opt, '"') != NULL ||
> strchr(ctl->date_opt, '`') != NULL ||
> - strstr(ctl->date_opt, "$(") != NULL) {
> + strchr(ctl->date_opt, '$') != NULL) {
> warnx(_
> ("The value of the --date option is not a valid date.\n"
> "In particular, it contains illegal character(s)."));
> @@ -1031,9 +1032,7 @@ calculate_adjustment(const struct hwclock_control *ctl,
> exact_adjustment =
> ((double)(systime - last_time)) * factor / (24 * 60 * 60)
> + not_adjusted;
> - tdrift_p->tv_sec = (int) exact_adjustment;
> - if (exact_adjustment < 0)
> - tdrift_p->tv_sec--;
> + tdrift_p->tv_sec = (int) floor(exact_adjustment);
Is the cast required now?
> tdrift_p->tv_usec = (exact_adjustment -
> (double)tdrift_p->tv_sec) * 1E6;
> if (ctl->debug) {
> @@ -1052,7 +1051,8 @@ calculate_adjustment(const struct hwclock_control *ctl,
> * But if the contents are clean (unchanged since read from disk), don't
> * bother.
> */
> -static void save_adjtime(const struct hwclock_control *ctl, const struct adjtime *adjtime)
> +static void save_adjtime(const struct hwclock_control *ctl,
> + const struct adjtime *adjtime)
> {
> char *content; /* Stuff to write to disk file */
> FILE *fp;
> @@ -1181,7 +1181,7 @@ static void determine_clock_access_method(const struct hwclock_control *ctl)
> * Return rc == 0 if everything went OK, rc != 0 if not.
> */
> static int
> -manipulate_clock(const struct hwclock_control *ctl, time_t set_time,
> +manipulate_clock(const struct hwclock_control *ctl, const time_t set_time,
> const struct timeval startup_time, struct adjtime *adjtime)
> {
> /* The time at which we read the Hardware Clock */
> -- snip
Changes look good Sami, thank you.
>
> There are replies to feedback below. I can only hope your email client is
> helping to find them as this message is a bit long.
>
>> commit c7a8f56920c738a90038cd6baaaec676c642b926
>> Author: Sami Kerola <kerolasa@iki.fi>
>> Date: Sat Dec 31 21:29:27 2016 +0000
>>
>> hwclock: remove trailing dot from messages that include system error message
>>
>> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>>>
>>> Nice, thank you.
>>>
>>> FWIW, I think this should apply to all message strings. (Apparently these
>>> types of changes annoy translators :)
>>>
>
> Correct. That is why I left most of the strings alone, but the few that
> ended with dot and had trailing system error output were too ugly. With
> a dot they look like:
>
> hwclock: important message.: system error
>
> That 'trailing' dot is not at the end of the line, but before : making it
> to be untolerable.
>
I wasn't objecting. I understand the reason and think that your change is
good. I was only commenting that I'd like to see more of it ;)
8< -----
>>
>>
>> commit d9ccb70cafaac62fd6c8294fbba3b3ab385d3537
>> Author: Sami Kerola <kerolasa@iki.fi>
>> Date: Tue Jul 26 10:52:15 2016 +0100
>>
>> hwclock: try RTCGET and RTCSET only when normal rtc fails
>>
>> The RTCGET and RTCSET are in use for sparcs with sbus, so try them as
>> fallback rather than always.
>>
>> Reference: https://github.com/torvalds/linux/blob/master/fs/compat_ioctl.c#L967-L974
>> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>>
>> diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
>> index fc1dfc3..9a67027 100644
>> --- a/sys-utils/hwclock-rtc.c
>> +++ b/sys-utils/hwclock-rtc.c
>> @@ -158,28 +158,31 @@ static int do_rtc_read_ioctl(int rtc_fd, struct tm *tm)
>> {
>> int rc = -1;
>> char *ioctlname;
>> -
>> #ifdef __sparc__
>> /* some but not all sparcs use a different ioctl and struct */
>> struct sparc_rtc_time stm;
>> +#endif
>>
>> 8< -------
>>
>>>
>>> Can't this be in the sparc ifdef just after this?
>>>
>>> Otherwise looks OK, thank you.
>>>
>
> That would mix declaration and code. Kept as is.
You mean keeping declarations at the top, I see. We break that rule
sometimes. I think the code would look nicer in this case. But I don't
have a strong opinion about it. It's fine your way.
>
>> commit 08e959db08840871979b00036d85aa6941f9fa76
>> Author: Sami Kerola <kerolasa@iki.fi>
>> Date: Tue Jul 26 08:54:00 2016 +0100
>>
>> hwclock: stream line synchronize_to_clock_tick_rtc()
>>
>> Flip if clauses to hit common case first. This should be easier and quicker
>> to read and run.
>>
>> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>>
>> diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
>> index f54e263..fc1dfc3 100644
>> --- a/sys-utils/hwclock-rtc.c
>> +++ b/sys-utils/hwclock-rtc.c
>>
>> 8< -------
>>
>> @@ -287,26 +276,34 @@ static int synchronize_to_clock_tick_rtc(const struct hwclock_control *ctl)
>> tv.tv_sec = 10;
>> tv.tv_usec = 0;
>> rc = select(rtc_fd + 1, &rfds, NULL, NULL, &tv);
>> - ret = 1;
>> - if (rc == -1)
>> - warn(_("select() to %s to wait for clock tick failed"),
>> - rtc_dev_name);
>> + if (0 < rc)
>> + ret = 0;
>> else if (rc == 0) {
>> if (ctl->debug)
>> printf(_("select() to %s to wait for clock tick timed out"),
>> rtc_dev_name);
>> } else
>> - ret = 0;
>> + warn(_("select() to %s to wait for clock tick failed"),
>> + rtc_dev_name);
>> /* Turn off update interrupts */
>> rc = ioctl(rtc_fd, RTC_UIE_OFF, 0);
>> if (rc == -1)
>> warn(_("ioctl() to %s to turn off update interrupts failed"),
>> rtc_dev_name);
>>
>>>
>>>
>>> The /* Turn off update interrupts */ code was the success block. You
>>> didn't move it up to your new success block at 'if (0 < rc)'. The
>>> interrupts will never be turned off with this patch.
>>>
>>>
>>
>> 8< -------
>
> I looked this change, and especially result of the change, again as I
> could not anymore tell what that diff does. This did indeed need change;
> ioctl returns not -1 when it is successful, so I am checking that rather
> than if it returns 0. Content of the 'success' block is left asis.
>
> Notice that I did not include your review signature to this change because
> I it does not sound you would be too happy about that before another look.
>
Doh, again I missed that "} else" was a shorthand statement. I seemed to
be blind to them that day. Of course we want the interrupts turned off
whether select() succeeds or fails. Sorry, my bad.
So this looks good to me, including your new change. Thank you.
8< ---------
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-08 9:39 ` Sami Kerola
@ 2017-01-08 21:21 ` J William Piggott
0 siblings, 0 replies; 28+ messages in thread
From: J William Piggott @ 2017-01-08 21:21 UTC (permalink / raw)
To: kerolasa; +Cc: util-linux
On 01/08/2017 04:39 AM, Sami Kerola wrote:
> On 7 January 2017 at 23:06, Sami Kerola <kerolasa@iki.fi> wrote:
>> On Sat, 7 Jan 2017, J William Piggott wrote:
>> git://github.com/kerolasa/lelux-utiliteetit.git hwclock-jwp-reviewed
>>
>> that has in almost all changes 'Reviewed-by: J William Piggott
>> <elseifthen@gmx.com>'. Notice also that the latest update is not 100%
>> ready. We have question of --compare open, and 'hwclock: move command-line
>> options to control structure' has issue that is not yet corrected yet.
>
> hwclock-jwp-reviewed got update:
>
> -- snip
> diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
> index b41acd450..c3bcc2fa0 100644
> --- a/sys-utils/hwclock.c
> +++ b/sys-utils/hwclock.c
> @@ -1245,11 +1245,6 @@ manipulate_clock(const struct hwclock_control
> *ctl, const time_t set_time,
> adjtime->dirty = TRUE;
> }
>
> - if (ctl->noadjfile || (ctl->systz && (ctl->utc || ctl->local_opt))) {
> - /* A little trick to avoid writing the file if we don't have to */
> - adjtime->dirty = FALSE;
> - }
> -
> if (ctl->show || ctl->get || ctl->adjust || ctl->hctosys
> || (!ctl->noadjfile && !ctl->systz && !ctl->predict)) {
> /* data from HW-clock are required */
> @@ -1558,7 +1553,7 @@ int main(int argc, char **argv)
> {
> struct hwclock_control ctl = { 0 };
> struct timeval startup_time;
> - struct adjtime adjtime;
> + struct adjtime adjtime = { 0 };
> /*
> * The time we started up, in seconds into the epoch, including
> * fractions.
> @@ -1839,8 +1834,12 @@ int main(int argc, char **argv)
> }
> }
>
> - if ((rc = read_adjtime(&ctl, &adjtime)) != 0)
> - hwclock_exit(&ctl, rc);
> + if (!ctl.noadjfile && !(ctl.systz && (ctl.utc || ctl.local_opt))) {
> + if ((rc = read_adjtime(&ctl, &adjtime)) != 0)
> + hwclock_exit(&ctl, rc);
> + } else
> + /* avoid writing adjtime file if we don't have to */
> + adjtime.dirty = FALSE;
> ctl.universal = hw_clock_is_utc(&ctl, adjtime);
> if (ctl.compare) {
> if (compare_clock(&ctl))
> -- snip
>
> that should fix:
Yes, this looks good I think.
The contents of the added if/else statements don't look indented though.
I like your improved comment, you might want to capitalize it and use
a period.
8< ----
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-08 10:09 ` Sami Kerola
@ 2017-01-08 21:21 ` J William Piggott
0 siblings, 0 replies; 28+ messages in thread
From: J William Piggott @ 2017-01-08 21:21 UTC (permalink / raw)
To: kerolasa; +Cc: util-linux
On 01/08/2017 05:09 AM, Sami Kerola wrote:
> On 7 January 2017 at 23:06, Sami Kerola <kerolasa@iki.fi> wrote:
>> On Sat, 7 Jan 2017, J William Piggott wrote:
>>> commit 509ac719dbe1bcc4a761f694497493525da39e25
>>> Author: Sami Kerola <kerolasa@iki.fi>
>>> Date: Sun Jul 17 09:09:33 2016 +0100
>>>
>>> docs: add instructions how to avoid alpha epoc Y2020 bug
>>>
>>> Recommend setting epoch using command line to for Alpha to avoid
>>> autodetection that is doomed to afail after 2020.
>>>
>>> Reference: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/alpha/kernel/rtc.c?id=85d0b3a573d8b711ee0c96199ac24a0f3283ed68
>>> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>>>
>>> diff --git a/sys-utils/hwclock-cmos.c b/sys-utils/hwclock-cmos.c
>>> index 9d9a42a..cf26064 100644
>>> --- a/sys-utils/hwclock-cmos.c
>>> +++ b/sys-utils/hwclock-cmos.c
>>> @@ -203,15 +203,16 @@ void set_cmos_epoch(const struct hwclock_control *ctl)
>>> #endif
>>>
>>> /*
>>> - * The kernel source today says: read the year.
>>> + * The automatic epoc determination in kernel
>>> + * linux/arch/alpha/kernel/rtc.c
>>> *
>>> * If it is in 0-19 then the epoch is 2000.
>>> * If it is in 20-47 then the epoch is 1980.
>>> * If it is in 48-69 then the epoch is 1952.
>>> * If it is in 70-99 then the epoch is 1928.
>>> *
>>> - * Otherwise the epoch is 1900.
>>> - * TODO: Clearly, this must be changed before 2019.
>>> + * Alpha users should explicitly define epoc using rtc kernel module
>>> + * option rtc epoc=<year>
>>> */
>>> /*
>>> * See whether we are dealing with SRM or MILO, as they have
>>> diff --git a/sys-utils/hwclock.8.in b/sys-utils/hwclock.8.in
>>> index 9534c19..de68e43 100644
>>> --- a/sys-utils/hwclock.8.in
>>> +++ b/sys-utils/hwclock.8.in
>>> @@ -466,6 +466,14 @@ For example, on a Digital Unix machine:
>>> .IP "" 4
>>> .B hwclock\ \-\-setepoch\ \-\-epoch=1952
>>> .RE
>>> +.IP
>>> +Beginning January 1, 2020 automatic epoc determination in kernel for systems
>>> +using 1900 will fail. In these cases it is recommended to specify epoc
>>> +explicitly with kernel module option.
>>> +.RS
>>> +.IP "" 4
>>> +.B options rtc epoch=1900
>>> +.RE
>>> .
>>> .TP
>>> .B \-\-funky\-toy
>>>
>>>
>>>>
>>>> I don't like this one because:
>>>>
>>>> Despite Richard's "doomed to fail" comment, I don't think util-linux
>>>> should document that as a fact. The heuristics could be updated. If the
>>>> kernel is still supporting DEC in 2019 it seems like they should
>>>> update them. It would be more work to remove the code than to adjust
>>>> the numbers.
>>>>
>>>> Also the patch has some errors:
>>>>
>>>> * The possible failure applies to more than 'systems using 1900'.
>>>>
>>>> * The Alpha rtc driver cannot be built as a module; I believe
>>>> epoch={year} is a kernel command line argument only.
>>>>
>>>> * The heuristics in drivers/char/rtc.c are slightly different than
>>>> what the patch references in linux/arch/alpha/kernel/rtc.c. I don't
>>>> know which one wins. They both differ from how they are described in
>>>> the hwclock-cmos.c comments, from which this patch deletes the 1900
>>>> possibility.
>>>>
>>>> The Alpha auto epoch detect has nothing to do with hwclock --setepoch
>>>> --epoch {year}. That is all done via rtc and can only set it manually.
>>>> Auto detecting the epoch only affects using --directisa on Alpha
>>>> machines. Direct access requires hwclock to handle the epoch offset
>>>> internally, because we're bypassing the kernel. So hwclock-cmos.c has
>>>> to figure out what to use. It tries heuristics and, ironically, it
>>>> tries to use the kernel's auto detect via the rtc driver. Why use
>>>> direct access if you have a working rtc interface!
>>>>
>>>> It is undocumented, but when using --directisa on alpha --epoch {year}
>>>> can be used to bypass auto detect. So --epoch {year} would be used for
>>>> all functions. The man-page says it can only be used with --setepoch.
>>>> I have no way to test this, so until now I have not documented it.
>>>>
>>
>> I left the change as-is for now, making the branch in it's current state
>> not to be ready. How about if I read carefully what you said above,
>> rethink what I thought earlier, and reply later.
>
> This change is dropped.
>
> What comes to other comments nice to hear 'doomed to fail' is not quite
> as bad situation as it was made to sound. Maybe it would be appropriate to
> deal alpha 2020 issue separately, with kernel upstream alpha maintainers.
>
> Right now my only recommendation is to clear out the situation this year
> (2017). After all 2020 is just round corner. We shall see whether clearning
> out means code and docs changes to kernel, hwclock, or both.
>
I have a solution to suggest for hwclock. I will write about it, but not
today.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-07 23:06 ` Sami Kerola
` (2 preceding siblings ...)
2017-01-08 21:21 ` J William Piggott
@ 2017-01-09 11:32 ` Karel Zak
2017-01-09 13:53 ` J William Piggott
3 siblings, 1 reply; 28+ messages in thread
From: Karel Zak @ 2017-01-09 11:32 UTC (permalink / raw)
To: Sami Kerola; +Cc: J William Piggott, util-linux
On Sat, Jan 07, 2017 at 11:06:23PM +0000, Sami Kerola wrote:
> > >
> > >
> > > I tried to convince Karel of this long ago. This code has been broken
> > > from day one, so nobody can be using it. IMO, it should be removed now.
> > >
> > >
>
> I agree. --compare is broken. I think there are couple options.
>
> 1) remove --compare code, and mention in manual page it's gone
> 2) keep the code, but deprecate it
> 3) keep the code, and fix it (but how???)
> 4) keep the broken code and whistle happy days theme song
> 5) something else
>
> I am 60-40 leaning towards recommending removal. If we don't remove rest
> of my 40 goes to deprecate. Karel, I think we need maintainer advice.
I have re-read our old discussions about this topic and I agree that
it's better to kill the problematic (broken?) functionality that care
about backward compatibility. It really seems nobody uses this stuff.
So, go ahead and send patch to remove all around --compare.
BTW, we had discussion about unmaintained adjtimex merge to
util-linux.
Karel
--
Karel Zak <kzak@redhat.com>
http://karelzak.blogspot.com
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-09 11:32 ` Karel Zak
@ 2017-01-09 13:53 ` J William Piggott
2017-01-09 20:48 ` Bjarni Ingi Gislason
0 siblings, 1 reply; 28+ messages in thread
From: J William Piggott @ 2017-01-09 13:53 UTC (permalink / raw)
To: Karel Zak, Sami Kerola; +Cc: util-linux
On 01/09/2017 06:32 AM, Karel Zak wrote:
> On Sat, Jan 07, 2017 at 11:06:23PM +0000, Sami Kerola wrote:
>>>>
>>>>
>>>> I tried to convince Karel of this long ago. This code has been broken
>>>> from day one, so nobody can be using it. IMO, it should be removed now.
>>>>
>>>>
>>
>> I agree. --compare is broken. I think there are couple options.
>>
>> 1) remove --compare code, and mention in manual page it's gone
>> 2) keep the code, but deprecate it
>> 3) keep the code, and fix it (but how???)
>> 4) keep the broken code and whistle happy days theme song
>> 5) something else
>>
>> I am 60-40 leaning towards recommending removal. If we don't remove rest
>> of my 40 goes to deprecate. Karel, I think we need maintainer advice.
>
> I have re-read our old discussions about this topic and I agree that
> it's better to kill the problematic (broken?) functionality that care
> about backward compatibility. It really seems nobody uses this stuff.
>
> So, go ahead and send patch to remove all around --compare.
>
> BTW, we had discussion about unmaintained adjtimex merge to
> util-linux.
>
> Karel
>
>
It is true, I suggested picking up the orphaned adjtimex(8) code as an
alternative to inserting it's functionality into hwclock(8). However, I
have since learned that the useful functions of adjtimex(8) are
available via the ntp utilities. The rest of its (dis)abilities have
the same problems we are seeing when reimplementing them into
hwclock(8). I think perhaps there were good reasons adjtimex(8) was
abandoned. Before, I believed adjtimex(8) might be a good fit for
util-linux, now I am not so sure. If someone demonstrates a useful
purpose for it I will change my position.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-09 13:53 ` J William Piggott
@ 2017-01-09 20:48 ` Bjarni Ingi Gislason
0 siblings, 0 replies; 28+ messages in thread
From: Bjarni Ingi Gislason @ 2017-01-09 20:48 UTC (permalink / raw)
To: J William Piggott; +Cc: Karel Zak, Sami Kerola, util-linux
On Mon, Jan 09, 2017 at 08:53:27AM -0500, J William Piggott wrote:
>
>
[...]
>
> It is true, I suggested picking up the orphaned adjtimex(8) code as an
> alternative to inserting it's functionality into hwclock(8). However, I
> have since learned that the useful functions of adjtimex(8) are
> available via the ntp utilities. The rest of its (dis)abilities have
> the same problems we are seeing when reimplementing them into
> hwclock(8). I think perhaps there were good reasons adjtimex(8) was
> abandoned. Before, I believed adjtimex(8) might be a good fit for
> util-linux, now I am not so sure. If someone demonstrates a useful
> purpose for it I will change my position.
>
>
I use "adjtimex" from the package in "Debian" to regulate the system
clock. "Debian" has made some changes and I too, that I have not yet sent
to them.
I do not use NTP (ntpdate-debian) except when the system clock deviates
more than 3 seconds from the right time. Then I update the system clock and
correct the hardware clock accordingly and let "hwclock" calculate a new
value for the drift (hwclock --systohc --update-drift).
I have added a option '-A' to my version of "adjtimex", to compare the
system and hardware clock about every 11 minutes (after a period of
different long pauses of comparisons) and change frequency and tick to keep
the system clock in compliance with the drift-corrected hardware clock.
My computer is only on for a few hours a day. The hardware clock is never
automatically updated on shutdown nor in the process of booting.
N.B. The WARNING is caused by not updating the hardware clock but relying on
the calculated drift to correct it. The offset of the system clock is now
-0.692 seconds. The hardware clock was last updated 1st June 2016.
The time between comparisons is 30 seconds multiplied by the first
Fibonacci numbers, excluding a repeated number.
Example of comparisons:
######## Start
1483984488 Mon Jan 9 17:54:48 UTC 2017
WARNING: CMOS time is 6.25 min behind system clock
--- current --- -- suggested --
cmos time system-cmos error_ppm tick freq tick freq
1483984488 0.005348 9999 4012975
1483984518 0.005172 -5.9 9999 4012975 9999 4397870
WARNING: CMOS time is 6.25 min behind system clock
--- current --- -- suggested --
cmos time system-cmos error_ppm tick freq tick freq
1483984519 0.002689 9999 4397870
1483984579 0.001900 -13.1 9999 4397870 9999 5259588
WARNING: CMOS time is 6.25 min behind system clock
--- current --- -- suggested --
cmos time system-cmos error_ppm tick freq tick freq
1483984580 0.004296 9999 5259588
1483984670 0.002953 -14.9 9999 5259588 9999 6237539
WARNING: CMOS time is 6.25 min behind system clock
--- current --- -- suggested --
cmos time system-cmos error_ppm tick freq tick freq
1483984671 0.009972 9999 6237539
1483984821 0.007474 -16.7 9999 6237539 10000 775398
WARNING: CMOS time is 6.25 min behind system clock
--- current --- -- suggested --
cmos time system-cmos error_ppm tick freq tick freq
1483984822 -0.005831 10000 775398
1483985062 0.005554 47.4 10000 775398 9999 4220208
WARNING: CMOS time is 6.25 min behind system clock
--- current --- -- suggested --
cmos time system-cmos error_ppm tick freq tick freq
1483985063 0.007414 9999 4220208
1483985453 0.000203 -18.5 9999 4220208 9999 5432026
WARNING: CMOS time is 6.25 min behind system clock
--- current --- -- suggested --
cmos time system-cmos error_ppm tick freq tick freq
1483985454 0.001917 9999 5432026
1483986084 0.005602 5.8 9999 5432026 9999 5048743
WARNING: CMOS time is 6.25 min behind system clock
--- current --- -- suggested --
cmos time system-cmos error_ppm tick freq tick freq
1483986085 0.016335 9999 5048743
1483986715 0.004416 -18.9 9999 5048743 9999 6288698
1483987345 0.023767 30.7 9999 6288698 9999 4275678
1483987975 0.027384 5.7 9999 4275678 9999 3899389
1483988605 0.015368 -19.1 9999 3899389 9999 5149314
1483989235 0.019079 5.9 9999 5149314 9999 4763352
1483989865 0.022661 5.7 9999 4763352 9999 4390684
1483990495 0.026288 5.8 9999 4390684 9999 4013452
1483991125 0.014275 -19.1 9999 4013452 9999 5263079
1483991755 0.033575 30.6 9999 5263079 9999 3255391
1483992385 0.021591 -19.0 9999 3255391 9999 4501993
1483993015 0.025241 5.8 9999 4501993 9999 4122356
The offset of the system clock is now -0.677 seconds.
--
Bjarni I. Gislason
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-08 21:21 ` J William Piggott
@ 2017-01-11 21:44 ` Sami Kerola
2017-01-13 1:30 ` J William Piggott
0 siblings, 1 reply; 28+ messages in thread
From: Sami Kerola @ 2017-01-11 21:44 UTC (permalink / raw)
To: J William Piggott; +Cc: util-linux
On Sun, 8 Jan 2017, J William Piggott wrote:
> > @@ -655,7 +655,8 @@ display_time(const bool hclock_valid, struct timeval hwctime)
> > * If anything goes wrong (and many things can), we return return code 10
> > * and arbitrary *time_p. Otherwise, return code is 0 and *time_p is valid.
> > */
> > -static int interpret_date_string(const struct hwclock_control *ctl, time_t * const time_p)
> > +static int interpret_date_string(const struct hwclock_control *ctl,
> > + time_t *const time_p)
>
>
> Sorry for nitpicking. I know you didn't write this, but as long as
> you're making a change here, shouldn't it be: const time_t *time_p?
Nope. That will make pointer value read-only, and that would cause
following error.
sys-utils/hwclock.c:720:11: error: assignment of read-only location '*time_p'
*time_p = seconds_since_epoch;
Code we have makes the pointer read-only, but leaves value writeable.
> > @@ -1031,9 +1032,7 @@ calculate_adjustment(const struct hwclock_control *ctl,
> > exact_adjustment =
> > ((double)(systime - last_time)) * factor / (24 * 60 * 60)
> > + not_adjusted;
> > - tdrift_p->tv_sec = (int) exact_adjustment;
> > - if (exact_adjustment < 0)
> > - tdrift_p->tv_sec--;
> > + tdrift_p->tv_sec = (int) floor(exact_adjustment);
>
> Is the cast required now?
floor(3) will return double, tv_sec is time_t. Yes I should and have
fixed the cast from int to time_t.
> >> @@ -158,28 +158,31 @@ static int do_rtc_read_ioctl(int rtc_fd, struct tm *tm)
> >> {
> >> int rc = -1;
> >> char *ioctlname;
> >> -
> >> #ifdef __sparc__
> >> /* some but not all sparcs use a different ioctl and struct */
> >> struct sparc_rtc_time stm;
> >> +#endif
> >>
> >> 8< -------
> >>
> >>>
> >>> Can't this be in the sparc ifdef just after this?
> >>>
> >>> Otherwise looks OK, thank you.
> >>>
> >
> > That would mix declaration and code. Kept as is.
>
> You mean keeping declarations at the top, I see. We break that rule
> sometimes. I think the code would look nicer in this case. But I don't
> have a strong opinion about it. It's fine your way.
Since C99 it is allowed to mix declarations and code, but quite franky I
find it irritating when declarations are hidden somewhere halfway down
function. Same story with #defines they should be at top of the file after
#includes before any functions - and no where else. IMHO these sort of
conventios make code reading, and understanding, easier.
-- snip
There is now new commit that removes --compare option, and all related
stuff. Naturally the earlier integer arithmetics change and deprecation in
manual page are removed from changes.
https://github.com/kerolasa/lelux-utiliteetit/commit/d3e33557e6794446c712ca264e54fe4b186a17ca
----------------------------------------------------------------
The following changes since commit 89958178f6d6ebe0944d423feaea66be521fff43:
If mtab support is disabled, disable ro/rw mtab checks (2017-01-10 17:32:32 +0100)
are available in the git repository at:
git://github.com/kerolasa/lelux-utiliteetit.git hwclock-jwp-reviewed
for you to fetch changes up to d3e33557e6794446c712ca264e54fe4b186a17ca:
hwclock: remove --compare option (2017-01-11 21:04:36 +0000)
----------------------------------------------------------------
Thank you JWP for reviews, I hope I did not miss anything but if I did let
me know and I will follow up. Even better if everyone agrees that this is
now good enough to be merged, and we can move forward with other updates
such as adjtimex(8) work.
--
Sami Kerola
http://www.iki.fi/kerolasa/
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-11 21:44 ` Sami Kerola
@ 2017-01-13 1:30 ` J William Piggott
2017-01-14 9:34 ` Sami Kerola
0 siblings, 1 reply; 28+ messages in thread
From: J William Piggott @ 2017-01-13 1:30 UTC (permalink / raw)
To: Sami Kerola; +Cc: util-linux
On 01/11/2017 04:44 PM, Sami Kerola wrote:
> On Sun, 8 Jan 2017, J William Piggott wrote:
>
>>> @@ -655,7 +655,8 @@ display_time(const bool hclock_valid, struct timeval hwctime)
>>> * If anything goes wrong (and many things can), we return return code 10
>>> * and arbitrary *time_p. Otherwise, return code is 0 and *time_p is valid.
>>> */
>>> -static int interpret_date_string(const struct hwclock_control *ctl, time_t * const time_p)
>>> +static int interpret_date_string(const struct hwclock_control *ctl,
>>> + time_t *const time_p)
>>
>>
>> Sorry for nitpicking. I know you didn't write this, but as long as
>> you're making a change here, shouldn't it be: const time_t *time_p?
>
> Nope. That will make pointer value read-only, and that would cause
> following error.
>
> sys-utils/hwclock.c:720:11: error: assignment of read-only location '*time_p'
> *time_p = seconds_since_epoch;
>
> Code we have makes the pointer read-only, but leaves value writeable.
>
I'm glad Karel built this forum with #include <dumb_question.h>
Thanks for the education and patience.
>>> @@ -1031,9 +1032,7 @@ calculate_adjustment(const struct hwclock_control *ctl,
>>> exact_adjustment =
>>> ((double)(systime - last_time)) * factor / (24 * 60 * 60)
>>> + not_adjusted;
>>> - tdrift_p->tv_sec = (int) exact_adjustment;
>>> - if (exact_adjustment < 0)
>>> - tdrift_p->tv_sec--;
>>> + tdrift_p->tv_sec = (int) floor(exact_adjustment);
>>
>> Is the cast required now?
>
> floor(3) will return double, tv_sec is time_t. Yes I should and have
> fixed the cast from int to time_t.
Isn't the cast implied by assignment and done automatically? Isn't that
why the previous mismatch with the (int) cast worked? In the next line
of code we assign a double to tv_usec which is a mismatch also.
I built your latest branch with and without the cast and the results
were binary identical.
8< -------
> ----------------------------------------------------------------
> The following changes since commit 89958178f6d6ebe0944d423feaea66be521fff43:
> If mtab support is disabled, disable ro/rw mtab checks (2017-01-10 17:32:32 +0100)
> are available in the git repository at:
> git://github.com/kerolasa/lelux-utiliteetit.git hwclock-jwp-reviewed
> for you to fetch changes up to d3e33557e6794446c712ca264e54fe4b186a17ca:
> hwclock: remove --compare option (2017-01-11 21:04:36 +0000)
> ----------------------------------------------------------------
>
> Thank you JWP for reviews, I hope I did not miss anything but if I did let
> me know and I will follow up. Even better if everyone agrees that this is
> now good enough to be merged, and we can move forward with other updates
> such as adjtimex(8) work.
Thanks for all the work, Sami. I've been testing your latest branch and
haven't found any problems. I cannot test all of your changes though, for
example I don't have libaudit installed. For what it's worth, I think it
is ready.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-13 1:30 ` J William Piggott
@ 2017-01-14 9:34 ` Sami Kerola
2017-01-14 22:51 ` J William Piggott
0 siblings, 1 reply; 28+ messages in thread
From: Sami Kerola @ 2017-01-14 9:34 UTC (permalink / raw)
To: J William Piggott; +Cc: util-linux
On Thu, 12 Jan 2017, J William Piggott wrote:
> On 01/11/2017 04:44 PM, Sami Kerola wrote:
> > On Sun, 8 Jan 2017, J William Piggott wrote:
> >>> @@ -1031,9 +1032,7 @@ calculate_adjustment(const struct hwclock_control *ctl,
> >>> exact_adjustment =
> >>> ((double)(systime - last_time)) * factor / (24 * 60 * 60)
> >>> + not_adjusted;
> >>> - tdrift_p->tv_sec = (int) exact_adjustment;
> >>> - if (exact_adjustment < 0)
> >>> - tdrift_p->tv_sec--;
> >>> + tdrift_p->tv_sec = (int) floor(exact_adjustment);
> >>
> >> Is the cast required now?
> >
> > floor(3) will return double, tv_sec is time_t. Yes I should and have
> > fixed the cast from int to time_t.
>
> Isn't the cast implied by assignment and done automatically? Isn't that
> why the previous mismatch with the (int) cast worked? In the next line
> of code we assign a double to tv_usec which is a mismatch also.
>
> I built your latest branch with and without the cast and the results
> were binary identical.
Preferably there should not be casts at all. But sometimes they are hard
to avoid, and then one should do as few casts as possible to avoid cutting
bits out.
Think casting in terms of shape sorting toy. The more data is casted
through none-matching types the more the original shape is modified to fit
to storage where it is pushed.
That is also the reason why casting to time_t in this case makes sense.
While it might be that right now, on systems we happen to use, int and
time_t are the same type there are no guarantees about other systems and
future. In practical terms; casting exactly to matching storage type is
the right way to avoid damaging data.
> > ----------------------------------------------------------------
> > The following changes since commit 89958178f6d6ebe0944d423feaea66be521fff43:
> > If mtab support is disabled, disable ro/rw mtab checks (2017-01-10 17:32:32 +0100)
> > are available in the git repository at:
> > git://github.com/kerolasa/lelux-utiliteetit.git hwclock-jwp-reviewed
> > for you to fetch changes up to d3e33557e6794446c712ca264e54fe4b186a17ca:
> > hwclock: remove --compare option (2017-01-11 21:04:36 +0000)
> > ----------------------------------------------------------------
> >
> > Thank you JWP for reviews, I hope I did not miss anything but if I did let
> > me know and I will follow up. Even better if everyone agrees that this is
> > now good enough to be merged, and we can move forward with other updates
> > such as adjtimex(8) work.
>
> Thanks for all the work, Sami. I've been testing your latest branch and
> haven't found any problems. I cannot test all of your changes though, for
> example I don't have libaudit installed. For what it's worth, I think it
> is ready.
Good point. I do not have libaudit either. Hearing that setup does not
have functional problems would really nice.
--
Sami Kerola
http://www.iki.fi/kerolasa/
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-14 9:34 ` Sami Kerola
@ 2017-01-14 22:51 ` J William Piggott
2017-01-22 19:03 ` J William Piggott
0 siblings, 1 reply; 28+ messages in thread
From: J William Piggott @ 2017-01-14 22:51 UTC (permalink / raw)
To: Sami Kerola; +Cc: util-linux
On 01/14/2017 04:34 AM, Sami Kerola wrote:
> On Thu, 12 Jan 2017, J William Piggott wrote:
>> On 01/11/2017 04:44 PM, Sami Kerola wrote:
>>> On Sun, 8 Jan 2017, J William Piggott wrote:
>>>>> @@ -1031,9 +1032,7 @@ calculate_adjustment(const struct hwclock_control *ctl,
>>>>> exact_adjustment =
>>>>> ((double)(systime - last_time)) * factor / (24 * 60 * 60)
>>>>> + not_adjusted;
>>>>> - tdrift_p->tv_sec = (int) exact_adjustment;
>>>>> - if (exact_adjustment < 0)
>>>>> - tdrift_p->tv_sec--;
>>>>> + tdrift_p->tv_sec = (int) floor(exact_adjustment);
>>>>
>>>> Is the cast required now?
>>>
>>> floor(3) will return double, tv_sec is time_t. Yes I should and have
>>> fixed the cast from int to time_t.
>>
>> Isn't the cast implied by assignment and done automatically? Isn't that
>> why the previous mismatch with the (int) cast worked? In the next line
>> of code we assign a double to tv_usec which is a mismatch also.
>>
>> I built your latest branch with and without the cast and the results
>> were binary identical.
>
> Preferably there should not be casts at all. But sometimes they are hard
> to avoid, and then one should do as few casts as possible to avoid cutting
> bits out.
>
> Think casting in terms of shape sorting toy. The more data is casted
> through none-matching types the more the original shape is modified to fit
> to storage where it is pushed.
>
> That is also the reason why casting to time_t in this case makes sense.
> While it might be that right now, on systems we happen to use, int and
> time_t are the same type there are no guarantees about other systems and
> future. In practical terms; casting exactly to matching storage type is
> the right way to avoid damaging data.
>
What I'm trying to say is, assignment operators automatically convert
the right operand to the type of the left operand; making your cast
unnecessary. Try it, build hwclock with the cast. Now, remove the cast
and build it. The resulting binaries are bit for bit identical.
ISO C11:
The type of an assignment expression is the type the left operand would have
after lvalue conversion.
In simple assignment (=), the value of the right operand is converted to
the type of the assignment expression and replaces the value stored in
the object designated by the left operand.
>>> ----------------------------------------------------------------
>>> The following changes since commit 89958178f6d6ebe0944d423feaea66be521fff43:
>>> If mtab support is disabled, disable ro/rw mtab checks (2017-01-10 17:32:32 +0100)
>>> are available in the git repository at:
>>> git://github.com/kerolasa/lelux-utiliteetit.git hwclock-jwp-reviewed
>>> for you to fetch changes up to d3e33557e6794446c712ca264e54fe4b186a17ca:
>>> hwclock: remove --compare option (2017-01-11 21:04:36 +0000)
>>> ----------------------------------------------------------------
>>>
>>> Thank you JWP for reviews, I hope I did not miss anything but if I did let
>>> me know and I will follow up. Even better if everyone agrees that this is
>>> now good enough to be merged, and we can move forward with other updates
>>> such as adjtimex(8) work.
>>
>> Thanks for all the work, Sami. I've been testing your latest branch and
>> haven't found any problems. I cannot test all of your changes though, for
>> example I don't have libaudit installed. For what it's worth, I think it
>> is ready.
>
> Good point. I do not have libaudit either. Hearing that setup does not
> have functional problems would really nice.
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-14 22:51 ` J William Piggott
@ 2017-01-22 19:03 ` J William Piggott
2017-01-25 21:54 ` Sami Kerola
0 siblings, 1 reply; 28+ messages in thread
From: J William Piggott @ 2017-01-22 19:03 UTC (permalink / raw)
To: Sami Kerola; +Cc: util-linux, Karel Zak
On 01/14/2017 05:51 PM, J William Piggott wrote:
>
>
> On 01/14/2017 04:34 AM, Sami Kerola wrote:
>> On Thu, 12 Jan 2017, J William Piggott wrote:
>>> On 01/11/2017 04:44 PM, Sami Kerola wrote:
>>>> On Sun, 8 Jan 2017, J William Piggott wrote:
>>>>>> @@ -1031,9 +1032,7 @@ calculate_adjustment(const struct hwclock_control *ctl,
>>>>>> exact_adjustment =
>>>>>> ((double)(systime - last_time)) * factor / (24 * 60 * 60)
>>>>>> + not_adjusted;
>>>>>> - tdrift_p->tv_sec = (int) exact_adjustment;
>>>>>> - if (exact_adjustment < 0)
>>>>>> - tdrift_p->tv_sec--;
>>>>>> + tdrift_p->tv_sec = (int) floor(exact_adjustment);
>>>>>
>>>>> Is the cast required now?
>>>>
>>>> floor(3) will return double, tv_sec is time_t. Yes I should and have
>>>> fixed the cast from int to time_t.
>>>
>>> Isn't the cast implied by assignment and done automatically? Isn't that
>>> why the previous mismatch with the (int) cast worked? In the next line
>>> of code we assign a double to tv_usec which is a mismatch also.
>>>
>>> I built your latest branch with and without the cast and the results
>>> were binary identical.
>>
>> Preferably there should not be casts at all. But sometimes they are hard
>> to avoid, and then one should do as few casts as possible to avoid cutting
>> bits out.
>>
>> Think casting in terms of shape sorting toy. The more data is casted
>> through none-matching types the more the original shape is modified to fit
>> to storage where it is pushed.
>>
>> That is also the reason why casting to time_t in this case makes sense.
>> While it might be that right now, on systems we happen to use, int and
>> time_t are the same type there are no guarantees about other systems and
>> future. In practical terms; casting exactly to matching storage type is
>> the right way to avoid damaging data.
>>
>
> What I'm trying to say is, assignment operators automatically convert
> the right operand to the type of the left operand; making your cast
> unnecessary. Try it, build hwclock with the cast. Now, remove the cast
> and build it. The resulting binaries are bit for bit identical.
>
> ISO C11:
> The type of an assignment expression is the type the left operand would have
> after lvalue conversion.
>
> In simple assignment (=), the value of the right operand is converted to
> the type of the assignment expression and replaces the value stored in
> the object designated by the left operand.
>
>
It wasn't the intent of my previous message to be offensive in any way,
Sami. I was only trying to state my position using different language,
so as not to simply repeat myself.
Also, I'm not continuing this topic to be argumentative. The reason is
for my own education. If my opinion is wrong, that is fine; I would like
to understand why, so as to improve my own coding.
As I further read the ISO C standard, I'm more convinced of my belief.
It seems to me the compiler is required to use the type conversion
routine associated with assignment operators, because it must yield two
different results. A /qualified/ left operand type is stored, but the
evaluation of the assignment expression returns an /unqualified/ left
operand type. Since using the assignment operator's type conversion
routine is required (if I'm correct), the compiler would ignore any cast
on the results of the right hand operand for code optimization, yes?
That seems to be true for my compiler, as the cast makes no difference
in the resulting binary.
If anyone knows the correct answer to this, I am very interested in
learning it. Thank you in advance.
>>>> ----------------------------------------------------------------
>>>> The following changes since commit 89958178f6d6ebe0944d423feaea66be521fff43:
>>>> If mtab support is disabled, disable ro/rw mtab checks (2017-01-10 17:32:32 +0100)
>>>> are available in the git repository at:
>>>> git://github.com/kerolasa/lelux-utiliteetit.git hwclock-jwp-reviewed
>>>> for you to fetch changes up to d3e33557e6794446c712ca264e54fe4b186a17ca:
>>>> hwclock: remove --compare option (2017-01-11 21:04:36 +0000)
>>>> ----------------------------------------------------------------
>>>>
>>>> Thank you JWP for reviews, I hope I did not miss anything but if I did let
>>>> me know and I will follow up. Even better if everyone agrees that this is
>>>> now good enough to be merged, and we can move forward with other updates
>>>> such as adjtimex(8) work.
>>>
>>> Thanks for all the work, Sami. I've been testing your latest branch and
>>> haven't found any problems. I cannot test all of your changes though, for
>>> example I don't have libaudit installed. For what it's worth, I think it
>>> is ready.
>>
>> Good point. I do not have libaudit either. Hearing that setup does not
>> have functional problems would really nice.
>>
> --
> 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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-22 19:03 ` J William Piggott
@ 2017-01-25 21:54 ` Sami Kerola
2017-01-27 2:07 ` J William Piggott
0 siblings, 1 reply; 28+ messages in thread
From: Sami Kerola @ 2017-01-25 21:54 UTC (permalink / raw)
To: J William Piggott; +Cc: util-linux, Karel Zak
On Sun, 22 Jan 2017, J William Piggott wrote:
> On 01/14/2017 05:51 PM, J William Piggott wrote:
> > On 01/14/2017 04:34 AM, Sami Kerola wrote:
> >> Preferably there should not be casts at all. But sometimes they are hard
> >> to avoid, and then one should do as few casts as possible to avoid cutting
> >> bits out.
> >>
> >> Think casting in terms of shape sorting toy. The more data is casted
> >> through none-matching types the more the original shape is modified to fit
> >> to storage where it is pushed.
> >>
> >> That is also the reason why casting to time_t in this case makes sense.
> >> While it might be that right now, on systems we happen to use, int and
> >> time_t are the same type there are no guarantees about other systems and
> >> future. In practical terms; casting exactly to matching storage type is
> >> the right way to avoid damaging data.
> >
> > What I'm trying to say is, assignment operators automatically convert
> > the right operand to the type of the left operand; making your cast
> > unnecessary. Try it, build hwclock with the cast. Now, remove the cast
> > and build it. The resulting binaries are bit for bit identical.
> >
> > ISO C11:
> > The type of an assignment expression is the type the left operand would have
> > after lvalue conversion.
> >
> > In simple assignment (=), the value of the right operand is converted to
> > the type of the assignment expression and replaces the value stored in
> > the object designated by the left operand.
>
> It wasn't the intent of my previous message to be offensive in any way,
> Sami. I was only trying to state my position using different language,
> so as not to simply repeat myself.
>
> Also, I'm not continuing this topic to be argumentative. The reason is
> for my own education. If my opinion is wrong, that is fine; I would like
> to understand why, so as to improve my own coding.
>
> As I further read the ISO C standard, I'm more convinced of my belief.
> It seems to me the compiler is required to use the type conversion
> routine associated with assignment operators, because it must yield two
> different results. A /qualified/ left operand type is stored, but the
> evaluation of the assignment expression returns an /unqualified/ left
> operand type. Since using the assignment operator's type conversion
> routine is required (if I'm correct), the compiler would ignore any cast
> on the results of the right hand operand for code optimization, yes?
>
> That seems to be true for my compiler, as the cast makes no difference
> in the resulting binary.
>
> If anyone knows the correct answer to this, I am very interested in
> learning it. Thank you in advance.
Hi William,
Sorry for late reply, I took a bit time off from reading / replying
emails.
Correct the cast that is in place now is technically unnecessary. Value of
that cast is to remind return value type and storage type differ. Such
assignments can, at least in theory, cause bugs so being explicit about
them is a good thing. As a matter of fact anything that makes code easier
to understand is welcome.
That said now I doubt whether 'technically unnecessary' cast is really a
thing that clarifies, but confuses code reader. Comments would be nice,
meanwhile I will think about this.
--
Sami Kerola
http://www.iki.fi/kerolasa/
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: pull: hwclock 27 changes
2017-01-25 21:54 ` Sami Kerola
@ 2017-01-27 2:07 ` J William Piggott
2017-02-02 15:04 ` [ping] Karel: " J William Piggott
0 siblings, 1 reply; 28+ messages in thread
From: J William Piggott @ 2017-01-27 2:07 UTC (permalink / raw)
To: Sami Kerola; +Cc: util-linux, Karel Zak
On 01/25/2017 04:54 PM, Sami Kerola wrote:
> On Sun, 22 Jan 2017, J William Piggott wrote:
>> On 01/14/2017 05:51 PM, J William Piggott wrote:
>>> On 01/14/2017 04:34 AM, Sami Kerola wrote:
>>>> Preferably there should not be casts at all. But sometimes they are hard
>>>> to avoid, and then one should do as few casts as possible to avoid cutting
>>>> bits out.
>>>>
>>>> Think casting in terms of shape sorting toy. The more data is casted
>>>> through none-matching types the more the original shape is modified to fit
>>>> to storage where it is pushed.
>>>>
>>>> That is also the reason why casting to time_t in this case makes sense.
>>>> While it might be that right now, on systems we happen to use, int and
>>>> time_t are the same type there are no guarantees about other systems and
>>>> future. In practical terms; casting exactly to matching storage type is
>>>> the right way to avoid damaging data.
>>>
>>> What I'm trying to say is, assignment operators automatically convert
>>> the right operand to the type of the left operand; making your cast
>>> unnecessary. Try it, build hwclock with the cast. Now, remove the cast
>>> and build it. The resulting binaries are bit for bit identical.
>>>
>>> ISO C11:
>>> The type of an assignment expression is the type the left operand would have
>>> after lvalue conversion.
>>>
>>> In simple assignment (=), the value of the right operand is converted to
>>> the type of the assignment expression and replaces the value stored in
>>> the object designated by the left operand.
>>
>> It wasn't the intent of my previous message to be offensive in any way,
>> Sami. I was only trying to state my position using different language,
>> so as not to simply repeat myself.
>>
>> Also, I'm not continuing this topic to be argumentative. The reason is
>> for my own education. If my opinion is wrong, that is fine; I would like
>> to understand why, so as to improve my own coding.
>>
>> As I further read the ISO C standard, I'm more convinced of my belief.
>> It seems to me the compiler is required to use the type conversion
>> routine associated with assignment operators, because it must yield two
>> different results. A /qualified/ left operand type is stored, but the
>> evaluation of the assignment expression returns an /unqualified/ left
>> operand type. Since using the assignment operator's type conversion
>> routine is required (if I'm correct), the compiler would ignore any cast
>> on the results of the right hand operand for code optimization, yes?
>>
>> That seems to be true for my compiler, as the cast makes no difference
>> in the resulting binary.
>>
>> If anyone knows the correct answer to this, I am very interested in
>> learning it. Thank you in advance.
>
> Hi William,
>
> Sorry for late reply, I took a bit time off from reading / replying
> emails.
No worries Sami. I thought you might have been angry at something I
wrote; it seem to unintentionally happen sometimes when talking across
cultural borders.
>
> Correct the cast that is in place now is technically unnecessary. Value of
> that cast is to remind return value type and storage type differ. Such
> assignments can, at least in theory, cause bugs so being explicit about
> them is a good thing. As a matter of fact anything that makes code easier
> to understand is welcome.
>
> That said now I doubt whether 'technically unnecessary' cast is really a
> thing that clarifies, but confuses code reader. Comments would be nice,
> meanwhile I will think about this.
>
I understood the previous symbolism of the cast; to draw attention to
dropping the fractional part of a variable. I don't perceive any value
in using it on the floor(3) function.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [ping] Karel: Re: pull: hwclock 27 changes
2017-01-27 2:07 ` J William Piggott
@ 2017-02-02 15:04 ` J William Piggott
2017-02-03 22:41 ` Sami Kerola
` (3 more replies)
0 siblings, 4 replies; 28+ messages in thread
From: J William Piggott @ 2017-02-02 15:04 UTC (permalink / raw)
To: Sami Kerola; +Cc: util-linux, Karel Zak
Karel,
I have some work to submit that applies on top of Sami's branch:
git://github.com/kerolasa/lelux-utiliteetit.git hwclock-jwp-reviewed
Outside of our ongoing discussion below regarding a 'technically
unnecessary' cast, I think Sami's work is ready for you to consider it
for committing. I would be interested in your thoughts on the cast.
On 01/26/2017 09:07 PM, J William Piggott wrote:
>
>
> On 01/25/2017 04:54 PM, Sami Kerola wrote:
>> On Sun, 22 Jan 2017, J William Piggott wrote:
>>> On 01/14/2017 05:51 PM, J William Piggott wrote:
>>>> On 01/14/2017 04:34 AM, Sami Kerola wrote:
>>>>> Preferably there should not be casts at all. But sometimes they are hard
>>>>> to avoid, and then one should do as few casts as possible to avoid cutting
>>>>> bits out.
>>>>>
>>>>> Think casting in terms of shape sorting toy. The more data is casted
>>>>> through none-matching types the more the original shape is modified to fit
>>>>> to storage where it is pushed.
>>>>>
>>>>> That is also the reason why casting to time_t in this case makes sense.
>>>>> While it might be that right now, on systems we happen to use, int and
>>>>> time_t are the same type there are no guarantees about other systems and
>>>>> future. In practical terms; casting exactly to matching storage type is
>>>>> the right way to avoid damaging data.
>>>>
>>>> What I'm trying to say is, assignment operators automatically convert
>>>> the right operand to the type of the left operand; making your cast
>>>> unnecessary. Try it, build hwclock with the cast. Now, remove the cast
>>>> and build it. The resulting binaries are bit for bit identical.
>>>>
>>>> ISO C11:
>>>> The type of an assignment expression is the type the left operand would have
>>>> after lvalue conversion.
>>>>
>>>> In simple assignment (=), the value of the right operand is converted to
>>>> the type of the assignment expression and replaces the value stored in
>>>> the object designated by the left operand.
>>>
>>> It wasn't the intent of my previous message to be offensive in any way,
>>> Sami. I was only trying to state my position using different language,
>>> so as not to simply repeat myself.
>>>
>>> Also, I'm not continuing this topic to be argumentative. The reason is
>>> for my own education. If my opinion is wrong, that is fine; I would like
>>> to understand why, so as to improve my own coding.
>>>
>>> As I further read the ISO C standard, I'm more convinced of my belief.
>>> It seems to me the compiler is required to use the type conversion
>>> routine associated with assignment operators, because it must yield two
>>> different results. A /qualified/ left operand type is stored, but the
>>> evaluation of the assignment expression returns an /unqualified/ left
>>> operand type. Since using the assignment operator's type conversion
>>> routine is required (if I'm correct), the compiler would ignore any cast
>>> on the results of the right hand operand for code optimization, yes?
>>>
>>> That seems to be true for my compiler, as the cast makes no difference
>>> in the resulting binary.
>>>
>>> If anyone knows the correct answer to this, I am very interested in
>>> learning it. Thank you in advance.
>>
>> Hi William,
>>
>> Sorry for late reply, I took a bit time off from reading / replying
>> emails.
>
> No worries Sami. I thought you might have been angry at something I
> wrote; it seem to unintentionally happen sometimes when talking across
> cultural borders.
>
>>
>> Correct the cast that is in place now is technically unnecessary. Value of
>> that cast is to remind return value type and storage type differ. Such
>> assignments can, at least in theory, cause bugs so being explicit about
>> them is a good thing. As a matter of fact anything that makes code easier
>> to understand is welcome.
>>
>> That said now I doubt whether 'technically unnecessary' cast is really a
>> thing that clarifies, but confuses code reader. Comments would be nice,
>> meanwhile I will think about this.
>>
>
> I understood the previous symbolism of the cast; to draw attention to
> dropping the fractional part of a variable. I don't perceive any value
> in using it on the floor(3) function.
>
>
> --
> 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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [ping] Karel: Re: pull: hwclock 27 changes
2017-02-02 15:04 ` [ping] Karel: " J William Piggott
@ 2017-02-03 22:41 ` Sami Kerola
2017-02-11 17:10 ` J William Piggott
2017-02-04 18:47 ` Karel Zak
` (2 subsequent siblings)
3 siblings, 1 reply; 28+ messages in thread
From: Sami Kerola @ 2017-02-03 22:41 UTC (permalink / raw)
To: J William Piggott; +Cc: util-linux, Karel Zak
On Thu, 2 Feb 2017, J William Piggott wrote:
> Karel,
> I have some work to submit that applies on top of Sami's branch:
>
> git://github.com/kerolasa/lelux-utiliteetit.git hwclock-jwp-reviewed
Nice to hear.
> Outside of our ongoing discussion below regarding a 'technically
> unnecessary' cast, I think Sami's work is ready for you to consider it
> for committing. I would be interested in your thoughts on the cast.
I had a Friday conversation with my work colleagues and majority thought
that 'unnecessary cast' is good thing, as explicit a cast is visual
reminder of type change.
We did consider maintenance burden of these casts, but counter argument
was found to be difficult to object. It is true that unnecessary casts
cause unnecessary change when either function return value or storage unit
changes - but such changes do not happen often. Therefore weight of visual
reminder is heavier than an attempt to keep potential future diffs
minimal.
With one thing everyone agreed, these sorts of little things should not
get in way too much. In short cast or don't but do not get stuck with a
detail like this. To be honest this is probably the best advice. While I
would see these sorts of type change reminders I'm happy to rip them out
if that is needed to move forward.
p.s. Branch hwclock-jwp-reviewed in my git is rebased on top of most
recent origin/master.
--
Sami Kerola
http://www.iki.fi/kerolasa/
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [ping] Karel: Re: pull: hwclock 27 changes
2017-02-02 15:04 ` [ping] Karel: " J William Piggott
2017-02-03 22:41 ` Sami Kerola
@ 2017-02-04 18:47 ` Karel Zak
2017-02-05 22:37 ` Sami Kerola
2017-02-09 10:43 ` Karel Zak
2017-02-09 11:09 ` Karel Zak
3 siblings, 1 reply; 28+ messages in thread
From: Karel Zak @ 2017-02-04 18:47 UTC (permalink / raw)
To: J William Piggott; +Cc: Sami Kerola, util-linux
On Thu, Feb 02, 2017 at 10:04:13AM -0500, J William Piggott wrote:
> Karel,
> I have some work to submit that applies on top of Sami's branch:
>
> git://github.com/kerolasa/lelux-utiliteetit.git hwclock-jwp-reviewed
I reviewed the patches 4 day ago and will merge it next week.
Karel
--
Karel Zak <kzak@redhat.com>
http://karelzak.blogspot.com
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [ping] Karel: Re: pull: hwclock 27 changes
2017-02-04 18:47 ` Karel Zak
@ 2017-02-05 22:37 ` Sami Kerola
0 siblings, 0 replies; 28+ messages in thread
From: Sami Kerola @ 2017-02-05 22:37 UTC (permalink / raw)
To: Karel Zak; +Cc: J William Piggott, util-linux
On Sat, 4 Feb 2017, Karel Zak wrote:
> On Thu, Feb 02, 2017 at 10:04:13AM -0500, J William Piggott wrote:
> > Karel,
> > I have some work to submit that applies on top of Sami's branch:
> >
> > git://github.com/kerolasa/lelux-utiliteetit.git hwclock-jwp-reviewed
>
> I reviewed the patches 4 day ago and will merge it next week.
Good to hear.
BTW when doing various checks I noticed clang failed to link with -lm.
Something with isnan() must be culprit, but I did not do further
investigation, simply changing $(MATH_LIBS) to -lm seems to work fine and
is part of my remote.
--
Sami Kerola
http://www.iki.fi/kerolasa/
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [ping] Karel: Re: pull: hwclock 27 changes
2017-02-02 15:04 ` [ping] Karel: " J William Piggott
2017-02-03 22:41 ` Sami Kerola
2017-02-04 18:47 ` Karel Zak
@ 2017-02-09 10:43 ` Karel Zak
2017-02-09 11:09 ` Karel Zak
3 siblings, 0 replies; 28+ messages in thread
From: Karel Zak @ 2017-02-09 10:43 UTC (permalink / raw)
To: J William Piggott; +Cc: Sami Kerola, util-linux
On Thu, Feb 02, 2017 at 10:04:13AM -0500, J William Piggott wrote:
> git://github.com/kerolasa/lelux-utiliteetit.git hwclock-jwp-reviewed
Applied, thanks guys!
Karel
--
Karel Zak <kzak@redhat.com>
http://karelzak.blogspot.com
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [ping] Karel: Re: pull: hwclock 27 changes
2017-02-02 15:04 ` [ping] Karel: " J William Piggott
` (2 preceding siblings ...)
2017-02-09 10:43 ` Karel Zak
@ 2017-02-09 11:09 ` Karel Zak
2017-02-11 17:10 ` J William Piggott
3 siblings, 1 reply; 28+ messages in thread
From: Karel Zak @ 2017-02-09 11:09 UTC (permalink / raw)
To: J William Piggott; +Cc: Sami Kerola, util-linux
On Thu, Feb 02, 2017 at 10:04:13AM -0500, J William Piggott wrote:
> Outside of our ongoing discussion below regarding a 'technically
> unnecessary' cast, I think Sami's work is ready for you to consider it
> for committing. I would be interested in your thoughts on the cast.
I have no strong opinion about it, for the reader it could be really
nice to follow code author ideas, but personally I think it's better
to not use it too often :-)
BTW, often bug in libblkid/fdisk is math without casts, something
like:
uint64_t x;
uint32_t a, b;
x = a + b
here it's better to be explicit and use (uint64) a + b. The same
problem if you write macros to hide some operations (we had this
problem in include/bitops.h).
Karel
--
Karel Zak <kzak@redhat.com>
http://karelzak.blogspot.com
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [ping] Karel: Re: pull: hwclock 27 changes
2017-02-03 22:41 ` Sami Kerola
@ 2017-02-11 17:10 ` J William Piggott
0 siblings, 0 replies; 28+ messages in thread
From: J William Piggott @ 2017-02-11 17:10 UTC (permalink / raw)
To: Sami Kerola; +Cc: util-linux, Karel Zak
On 02/03/2017 05:41 PM, Sami Kerola wrote:
> On Thu, 2 Feb 2017, J William Piggott wrote:
>
>> Karel,
>> I have some work to submit that applies on top of Sami's branch:
>>
>> git://github.com/kerolasa/lelux-utiliteetit.git hwclock-jwp-reviewed
>
> Nice to hear.
>
>> Outside of our ongoing discussion below regarding a 'technically
>> unnecessary' cast, I think Sami's work is ready for you to consider it
>> for committing. I would be interested in your thoughts on the cast.
>
> I had a Friday conversation with my work colleagues and majority thought
> that 'unnecessary cast' is good thing, as explicit a cast is visual
> reminder of type change.
>
> We did consider maintenance burden of these casts, but counter argument
> was found to be difficult to object. It is true that unnecessary casts
> cause unnecessary change when either function return value or storage unit
> changes - but such changes do not happen often. Therefore weight of visual
> reminder is heavier than an attempt to keep potential future diffs
> minimal.
>
> With one thing everyone agreed, these sorts of little things should not
> get in way too much. In short cast or don't but do not get stuck with a
> detail like this. To be honest this is probably the best advice. While I
> would see these sorts of type change reminders I'm happy to rip them out
> if that is needed to move forward.
If I understand correctly, your position is that this too trivial to be
wasting time talking about. I agree, and disagree.
Using this technique in this way does trivialize it; I believe that is
in itself harmful. I will elaborate in my reply to Karel.
>
> p.s. Branch hwclock-jwp-reviewed in my git is rebased on top of most
> recent origin/master.
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [ping] Karel: Re: pull: hwclock 27 changes
2017-02-09 11:09 ` Karel Zak
@ 2017-02-11 17:10 ` J William Piggott
0 siblings, 0 replies; 28+ messages in thread
From: J William Piggott @ 2017-02-11 17:10 UTC (permalink / raw)
To: Karel Zak; +Cc: Sami Kerola, util-linux
On 02/09/2017 06:09 AM, Karel Zak wrote:
> On Thu, Feb 02, 2017 at 10:04:13AM -0500, J William Piggott wrote:
>> Outside of our ongoing discussion below regarding a 'technically
>> unnecessary' cast, I think Sami's work is ready for you to consider it
>> for committing. I would be interested in your thoughts on the cast.
>
> I have no strong opinion about it, for the reader it could be really
> nice to follow code author ideas, but personally I think it's better
> to not use it too often :-)
At the beginning of this discussion I did not have a strong opinion
either ... but now I seem to have found one:)
>
> BTW, often bug in libblkid/fdisk is math without casts, something
> like:
>
> uint64_t x;
> uint32_t a, b;
>
> x = a + b
>
> here it's better to be explicit and use (uint64) a + b. The same
> problem if you write macros to hide some operations (we had this
> problem in include/bitops.h).
This is a good tip to remember, and a good example of why using
these noop symbolic casts merely as a reminder that an assignment
operator is making an automatic type conversion is, IMO, harmful.
This 'x = (uint64) a + b' looks very much like one of those noop
symbolic casts; when in fact it is functional and important. I believe
that loading up code with these noop symbolic casts will condition
readers to gloss over them as: 'this is just one of those assignment
conversion reminders.'
In your example the cast has nothing to do with the assignment operator,
it is telling the (+) addition operator we need a different result from
it. It would be better to remove this ambiguity by placing the cast near
the operator that it is associated with. Like this:
x = a + (uint64) b
I think the technique of applying a noop cast has an important use, but
that it is only effective when use judiciously.
I will use Sami's first patch to remove the FLOOR macro as an example.
- tdrift_p->tv_sec = FLOOR(exact_adjustment);
+ tdrift_p->tv_sec = (int) exact_adjustment;
+ if (exact_adjustment < 0)
+ tdrift_p->tv_sec--;
Here is where I think these noop symbolic casts shine, it is not a
yellow flag reminder, but a red flag telling the reader that this
automatic type conversion by the assignment operator /is/ changing the
value /and/ this change is imperative for the application to work
properly. It is saying: you need to understand what is happening here.
If this technique is ever used as merely a cautionary flag, then it will
always be viewed that way, trivially; then when we really need it to
forcefully grab the readers attention ... it will not.
For myself the rule to use this technique would be:
Use a noop symbolic type cast to draw attention to an automatic type
conversion by an assignment operator when:
* the stored value is being changed by the conversion
AND
* the change is imperative to the application's operation.
One final comment with regard to the cast being discussed.
The origin of the floor(3) argument is a time_t, so we know the integer
part that it returns will fit in a time_t. There is no reason to add a
distracting cautionary sign. If the reader follows that sign it will
lead them to a dead end. After they have jumped down a few of these
rabbit holes they will begin to ignore these casts and anything
that looks like one, IMO.
>
> Karel
>
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2017-02-11 17:10 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-31 21:41 pull: hwclock 27 changes Sami Kerola
2017-01-03 14:34 ` J William Piggott
[not found] ` <05e4405f-8096-9261-f9f2-d2c6b84675bc@gmx.com>
2017-01-07 19:37 ` J William Piggott
2017-01-07 20:32 ` J William Piggott
2017-01-07 23:06 ` Sami Kerola
2017-01-08 9:39 ` Sami Kerola
2017-01-08 21:21 ` J William Piggott
2017-01-08 10:09 ` Sami Kerola
2017-01-08 21:21 ` J William Piggott
2017-01-08 21:21 ` J William Piggott
2017-01-11 21:44 ` Sami Kerola
2017-01-13 1:30 ` J William Piggott
2017-01-14 9:34 ` Sami Kerola
2017-01-14 22:51 ` J William Piggott
2017-01-22 19:03 ` J William Piggott
2017-01-25 21:54 ` Sami Kerola
2017-01-27 2:07 ` J William Piggott
2017-02-02 15:04 ` [ping] Karel: " J William Piggott
2017-02-03 22:41 ` Sami Kerola
2017-02-11 17:10 ` J William Piggott
2017-02-04 18:47 ` Karel Zak
2017-02-05 22:37 ` Sami Kerola
2017-02-09 10:43 ` Karel Zak
2017-02-09 11:09 ` Karel Zak
2017-02-11 17:10 ` J William Piggott
2017-01-09 11:32 ` Karel Zak
2017-01-09 13:53 ` J William Piggott
2017-01-09 20:48 ` Bjarni Ingi Gislason
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.