More tolerant parsing of /etc/adjtime

[alkisg]

My issue is this:
# timedatectl set-local-rtc 1
Failed to set local RTC: Failed to set RTC to local/UTC: Input/output error

This happens when /etc/adjtime is not exactly as systemd expects it.
While the reason for this is outside the systemd code, maybe systemd can be more tolerant and allow a few variations.

Actual examples that I've seen out there:

1. Missing new line at the end:

0.000000 1455647333 0.000000
1455647333
UTC no-newline-here

This problem is caused by
a) https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=699554
b) systemd.postinst on recent Debian/Ubuntu (I haven't yet filed a bug report for that one, will do so soon).

2. No line for UTC/LOCAL at all:

0.0 0 0.0
0

3. Only the first line exists:

0.0 0 0.0

I don't know which script causes examples (2) and (3), but I see them in a lot of Ubuntu 12.04 installations.

So if possible, make the /etc/adjtime parsing a bit more tolerant.
Thanks!

[martinpitt]

clock_is_localtime() is hard to test with an unit test right now, as it hardcodes the /etc/adjtime path. I'll see to making that testable.

The other half is in src/timedate/timedated.c (context_write_data_local_rtc()), but this is more approachable to black-box testing in the integration tests (i. e. modify the actual /etc/adjtime and call timedatectl).

[martinpitt]

martinpitt/systemd@6369641 makes clock_is_localtime() testable and adds some initial tests. It's now really simple to add new scenarios.

One thing that caught my eye is that an unknown value like "FOO" in the third line of /etc/adjtime is interpreted as "UTC", i. e. any value except "LOCAL" is. In hindsight this should probably have been -EIO instead, but I guess for the sake of backwards compatibility we keep that behaviour?

[keszybz]

I don't think we should keep backwards compatibility here. The documentation is clear, timedatectl(1) refers to hwclock(8) which says that it must be "UTC" or "LOCAL". Bug-for-bug compatibility of undocumented things was never preserved.

[martinpitt]

@keszybz: I figure clock_is_localtime() should still just return 0 in the case of an unknown value. However, timedatectl should probably not change an unknown value, but just error out (current master just changes the value regardless of what it is).

If you agree, I'll change the tests and code accordingly.

[keszybz]

should still just return 0 in the case of an unknown value.

Sounds reasonable. Forget my previous comment.

timedatectl should probably not change an unknown value

This sounds problematic. If the value is wrong, we should just allow setting a new value no matter what. I'd be cumbersome for users to have to first fix it to something that timedatectl will accept, and then use timedatectl to change it.

So... I think we should treat LOCAL in the third line to mean LOCAL time, and anything else as UTC. If the value is anything other than UTC or LOCAL and non-empty, we can issue a warning.

[martinpitt]

So... I think we should treat LOCAL in the third line to mean LOCAL time, and anything else as UTC.

That's indeed what currently happens.

If the value is anything other than UTC or LOCAL and non-empty, we can issue a warning.

in timedated's journal? (That's simple to do). Or somewhere else, like timedatectl's stderr? Then we need to communicate this warning to timedatectl, or that has to parse adjtime itself (which requires moving that code to src/shared/).

[keszybz]

I don't think it's worth the trouble to rearchitecture the code to produce a warning in timedatectl. I was thinking about timedated writing something to the journal, but meh, maybe things are fine as they are now.