Hello Lukas Wunner,
Thanks for following up with both a new proposal and patch!
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.
>
> (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)
>
> (4) If systemd is present, we don't adjust the timezone once more in
> hwclock-set.
>
> Does this work for you? I've already coded the above up in my local repo
> but haven't tested it yet, pending your response. (Tentative patch is
> attached.)
This sounds promising to me. (I have some nitpicks about your patch
inlined below though.)
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.)
I would very much welcome a wider review of this from interested parties
from their perspective. If you want to reach out to people that would be
very welcome help.... i.e. to some or all of
Debian sysvinit maintainers <pkg-sysvinit-de...@lists.alioth.debian.org>,
Debian systemd Maintainers <pkg-systemd-maintain...@lists.alioth.debian.org>,
Debian kernel team <debian-ker...@lists.debian.org>,
debian-de...@lists.debian.org
... and maybe somewhere else I didn't think of.
(FYI I've already mentioned the bug report and your suggestion on the
debian systemd maintainers irc channel to try to get them interested
and giving feedback already...)
>
> Thanks,
>
> Lukas
> diff --git a/debian/hwclock-set b/debian/hwclock-set
> index eacf948..516563c 100755
> --- a/debian/hwclock-set
> +++ b/debian/hwclock-set
> @@ -4,10 +4,6 @@
>
> dev=$1
>
> -if [ -e /run/systemd/system ] ; then
> - exit 0
> -fi
> -
> if [ -e /run/udev/hwclock-set ]; then
> exit 0
> fi
> @@ -20,17 +16,33 @@ fi
> BADYEAR=no
> HWCLOCKACCESS=yes
> HWCLOCKPARS=
> -HCTOSYS_DEVICE=rtc0
> +HCTOSYS_DEVICE=
This change makes the default inconsistent with /etc/init.d/hwclock.sh
and I don't see why it's needed. Can't we just drop the change?!
> if [ -f /etc/default/hwclock ] ; then
> . /etc/default/hwclock
> fi
>
> +if [ $HWCLOCKACCESS != yes -o \
> + ( -n $HCTOSYS_DEVICE -a $HCTOSYS_DEVICE != $2 ) ]
Please avoid -a and -o for pedantic checkbashism reasons, always quote
variables and (together with the previous) just use:
if [ "$HWCLOCKACCESS" != yes ] || [ "$HCTOSYS_DEVICE" != "$rtc" ) ]; then
Note: rtc=$2 should be added below dev=$1 at the top of the script...
> + exit 0
> +fi
> +
> +# If the kernel has already set the system time from the hwclock,
> +# we only adjust the timezone
> +cat /sys/class/rtc/*/hctosys 2>/dev/null | while read kernel_hctosys ; do
> + if [ $kernel_hctosys = 1 ] ; then
Please quote "$kernel_hctosys" in case of reading in something crazy/empty...
> + break
> + fi
> +done
(Maybe your way is more efficiant and not completely unreadable either,
so ok ... but what do you think about:
KERNEL_DID_HCTOSYS=no
if grep -q '^1$' /sys/class/rtc/*/hctosys 2>/dev/null ; then
KERNEL_DID_HCTOSYS=yes
fi
... plus change the condition wrapping 'hwclock ... --hctosys' below
to use the new more abstract/descriptive variable name? [1]. grep is
Essential: yes.)
> +
> if [ yes = "$BADYEAR" ] ; then
> - /sbin/hwclock --rtc=$dev --systz --badyear
> - /sbin/hwclock --rtc=$dev --hctosys --badyear
> -else
> - /sbin/hwclock --rtc=$dev --systz
> - /sbin/hwclock --rtc=$dev --hctosys
> + badyear="--badyear"
> +fi
(Thanks for this cleanup.)
> +
> +if [ ! -e /run/systemd/system ] ; then
> + /sbin/hwclock --rtc=$dev --systz $badyear
> +fi
> +if [ $kernel_hctosys != 1 ] ; then
(Footnote [1]: if [ "$KERNEL_DID_HCTOSYS" = no ]; then)
Again, please always quote variables.... (I might accept not quoting
some variables that are always explicitly initialized in the script
itself, but even then why not just quote it for consistency?)
> + /sbin/hwclock --rtc=$dev --hctosys $badyear
> fi
>
> # Note 'touch' may not be available in initramfs
(FYI, this initramfs comment is misleading. Might still be useful to
check with initramfs-tools maintainers that they have no intention of
ever adding hwclock to initramfs again though. For the historically
interested: The hwclock was only temporarily added to initramfs briefly
during a development cycle. Never in any officially released Debian
version. History in #763777, #763823, etc. Eventually e2fsck was
patched to avoid needing hwclock in initramfs. IIRC.)
> 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.
Regards,
Andreas Henriksson
