Re: pull: hwclock 27 changes

[Sami Kerola <kerolasa@iki.fi>]

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/