Hi Omair,
I have reservations about this but will let others comment.
Looking at the code, readEntireFile doesn't close the file if it returns
-ENOMEM. It also doesn't free the allocated memory if it returns -ENOMEM
or via the goto fail path. (I'm not a fan of this goto fail style either.)
Thanks,
David
On 26/08/2013 1:18 PM, Omair Majid wrote:
Hi David,
Thanks for looking at the patch!
On 08/25/2013 09:59 PM, David Holmes wrote:
On 24/08/2013 7:09 AM, Omair Majid wrote:
The algorithm that OpenJDK uses to guess the local timezone ID on Linux
goes like this:
1. If TZ environment variable is set, use that
2. If /etc/timezone is readable, read the time zone from there
3. If /etc/localtime is a symlink, resolve that, and use the name to
guess the time zone.
4. Scan /usr/share/zoneinfo for a file whose contents match the contents
of /etc/localtime.
Step 4 (if it is ever reached) is probably going to lead to incorrect
results since there are a number of timezones that have the same
zoneinfo data (such as Europe/London and Europe/Belfast). So it seems
sensible to me to try and use additional sources to guess the timezone
ID before resorting to file content comparisons.
The webrev adds a step between 2 and 3 that reads and parses
/etc/sysconfig/clock to extract the timezone:
http://cr.openjdk.java.net/~omajid/webrevs/timezone-read-sysconfig-clock/00/
This file exists on some Red Hat Enterprise Linux (and derivative)
distributions and contains contents that look this:
# The time zone of the system is defined by the contents of
/etc/localtime.
# This file is only for evaluation by system-config-date, do not rely
on its
# contents elsewhere.
ZONE="Europe/Zurich"
Surely the implication here is that if this file exists and has this
data then /etc/localtime should be correctly configured - hence we
should use that. ie at best this should come after step 3 not before it.
The systems that I was looking at where /etc/sysconfig/clock exists,
/etc/localtime is not a symlink. So step 3 is doesn't actually do
anything. Anyway, I can move this to after step 3 - I have no issues
with that.
But even then when someone writes "do not rely on its
contents elsewhere" I'm inclined to do what they suggest!
Yes, I would prefer to not use this file either, but Petr Machata (who
maintains these timezone related packages in Fedora) tells me it's the
closest thing to an authoritative source. In fact, on an update of the
timezone packages, a script already parses /etc/sysconfig/clock and
copies the zoneinfo file over to /etc/localtime [1].
Oh, and I forgot to mention that this is not a problem on newer
systemd-using Linux distributions (including Arch, Fedora and Gentoo)
where /etc/localtime is always a symlink. They do not carry this
/etc/sysconfig/clock file.
Thanks,
Omair
[1] https://bugzilla.redhat.com/show_bug.cgi?id=186742#c8
