* 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
[parent not found: <05e4405f-8096-9261-f9f2-d2c6b84675bc@gmx.com>]
* 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-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-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-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 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 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-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-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-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
* 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
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.