[Pkg-sysvinit-devel] Bug#855203: hwclock-set: Synchronize from hwclock despite systemd presence

[Lukas Wunner]

[cc += pkg-sysvinit-devel at lists.alioth.debian.org,
       pkg-systemd-maintainers at lists.alioth.debian.org,
       debian-kernel at lists.debian.org, debian-devel at lists.debian.org
       (as requested by Andreas Henriksson)]
[cc += #785445, #785419, Rick Thomas (bug reporter)]

Hi Andreas,

sorry for the delay.  Attached is a new patch to address your review
comments.  (Thanks a lot for them!)

The patch no longer merely fixes the issue relating to systemd presence,
but seeks to tackle the problem more comprehensively by making hwclock-set
behave identically to hwclock.sh.  That way, users can switch between
sysvinit and systemd-udevd and get the same behavior without having to
change their hwclock configuration or experiencing unpleasant surprises.


On Tue, Feb 28, 2017 at 10:20:24PM +0100, Andreas Henriksson wrote:
> On Mon, Feb 27, 2017 at 02:34:34PM +0100, Lukas Wunner wrote:
> > Okay, let's define a policy first.  How about:
> > 
> > (1) Users can set HWCLOCKACCESS=no in /etc/default/hwclock to prohibit
> >     setting the system time or time zone from any RTC.  (The parameter
> >     is already defined in the config file, but it's not honored by
> >     hwclock-set.)
> > 
> > (2) Users can set HCTOSYS_DEVICE in /etc/default/hwclock to constrain
> >     setting the system time or time zone to a specific RTC.  (Same as
> >     above, parameter is already defined but not honored by hwclock-set.
> >     This will also fix #785445.)  If this parameter is not set,
> >     hwclock-set will pick the first RTC that becomes available.

I've changed (2) to pick rtc0 (instead of the first RTC that becomes
available), otherwise I'd introduce a change of behavior (as you've
correctly pointed out).


> > (3) If the kernel has already set the system time from *any* hwclock,
> >     we don't set it once more in hwclock-set.  (We only adjust the
> >     timezone.)
> 
> (3. would unfix #785445 from 2. again though, right? Maybe also support
> HWCLOCKACCESS=force which skips 3. would really offer a solution
> option for #785445 ? Reminder: This might also need 'force' to be
> converted to simply 'yes' (default) in /etc/init.d/hwclock.sh)

Good catch.  I've changed (3) to not set the system time if the kernel
has already done so AND the kernel used the same clock that was chosen
by the user in HCTOSYS_DEVICE (or rtc0 if not set by the user).

Thus, in a situation as described in #785445, the user would configure
HCTOSYS_DEVICE=rtc1, thereby instructing hwclock-set to override the
unreliable rtc0 used by the kernel.  This is actually just like hwclock.sh
behaves (if udev is not used).

@Rick Thomas: Could you verify if the attached patch solves this issue
for you?


> Targeting this at Debian (doesn't suffer from the problem you're trying
> to fix in any officially supported system AFAIK) we'll need to be
> careful to not introduce regressions for any currently existing
> configuration. (There are unfortunately many (non-default) combinations
> to consider, like sysvinit, etc.)

The issue relating to systemd presence does affect Debian if the RTC's
driver is compiled as a module, which is true for these arches:

$ git clone https://anonscm.debian.org/git/kernel/linux.git
$ egrep -rc 'CONFIG_RTC_DRV.*=m$' linux/debian/config | egrep -v ':0$'
linux/debian/config/config:5
linux/debian/config/kernelarch-mips/config.malta:11
linux/debian/config/kernelarch-powerpc/config-arch-64-be:1
linux/debian/config/arm64/config:1

Agreed on being careful to avoid regressions though.


> > diff --git a/debian/hwclock.rules b/debian/hwclock.rules
> > index 972e4aa..8584deb 100644
> > --- a/debian/hwclock.rules
> > +++ b/debian/hwclock.rules
> > @@ -1,4 +1,4 @@
> >  # Set the System Time from the Hardware Clock and set the kernel's timezone
> >  # value to the local timezone when the kernel clock module is loaded.
> >  
> > -KERNEL=="rtc0", RUN+="/lib/udev/hwclock-set $root/$name"
> > +KERNEL=="rtc*", RUN+="/lib/udev/hwclock-set $root/$name $kernel"
> 
> In general taking the initial policy and inlining it as comments in the
> sources would be useful for future generations trying to figure out what
> the idea was when this was changes. Also rephrasing it as eg.
> # Don't touch timezone when running systemd, because .....
> The 'because ...' part makes it easy for future generations to evaluate
> if the the check is still valid.

I've added some code comments to hwclock-set to hopefully improve clarity,
however I'm skeptical about keeping old code as comments, the git history
should be sufficient for archeologists. :-)

All your other comments make total sense to me and I've amended hwclock-set
accordingly.  Let me know if there is something else you want changed or if
I've missed anything.

Thanks!

Lukas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-hwclock-set-Behave-like-hwclock.sh.patch
Type: text/x-diff
Size: 8070 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/pkg-sysvinit-devel/attachments/20170317/5741ab07/attachment.patch>