Comment 15 for bug 1152187

Revision history for this message
Allison Karlitskaya (desrt) wrote :

I've reviewed the review in comment #14 and looked over the code myself as well as talking to Lennart.

Point-by-point:

- I don't understand why having /etc/localtime as a symlink is anything but desirable other than that Debian didn't do it this way before. I do notice that the symlink is not replaced atomically (using the rename-temp trick), however, meaning that the system is (de facto) on UTC for a short period during changes. I've raised this point with upstream.

- The race condition between checking timezone validity and changing the timezone is a total non-issue; mostly for the reason that you mention, but also because even if the timezone was erased _after_ the link was established, the result would be the same: dangling symlink. This is really not a race...

- the story with hwclock_set_timezone() and hwclock_reset_timezone() is stupidly complicated because of Linux being ridiculous on this point. The kernel wants to know the timezone for two reasons: in case the RTC is in localtime, it wants to jog the system time (that it assumed was in UTC) to proper UTC. This is done only on the first call to settimeofday(). The kernel also wants to know the timezone for other reasons though (like setting proper timestamps on FAT filesystems). The way to set the timezone for these other reasons without doing the 'first time warp' is to first set a NULL timezone (no warp on the first time) and then make a second call to actually set the timezone. I agree that it's stupid that the kernel should want to know what timezone userspace thinks that it's in, but that's how it is...

- the write_data_local_rtc() point you raise is valid code, but utterly ugly. I asked Lennart to rewrite it.

- the wraparound issue: I assume you mean that relative time changes are not checked for wrapping the clock. I don't think this is a big problem since it's a privileged operation and you could simply say that this is the semantics of the interface (and honestly, I can't imagine what else you would want to happen in this situation -- we could end up with a clock set to a ridiculous time, but we could end up with that anyway).

- localised strings: true.

- Lennart claims that realloc() is smart enough not to reallocate every time so we'd be wasting our time by trying to duplicate its efforts. That claim seems a bit suspect from the standpoint of my doubt that it would waste as much as double the amount of requested memory, but I didn't investigate further. I don't believe this to be a substantial problem in any case.

- The examples you give for hostnames accepted by hostname_is_valid() are obviously pretty bogus. This will be fixed upstream.