[libvirt] [PATCH] util: fix virTimeLocalOffsetFromUTC DST processing
The original version of virTimeLocalOffsetFromUTC() (commit 1cddaea7aeca441b733c31990a3f139dd2d346f6) would fail for certain times of the day if daylight savings time was active. This could most easily be seen by uncommenting the TEST_LOCALOFFSET() cases that invlude a DST setting. After a lot of experimenting, I found that the way to solve it in almost all test cases is to set tm_isdst = -1 in the stuct tm prior to calling mktime(). Once this is done, the correct offset is returned for all test cases at all times except the two hours just after 00:00:00 Jan 1 UTC - during that time, any timezone that is *behind* UTC, and that is supposed to always be in DST will not have DST accounted for in its offset. I still do not know the source of this problem, but it appears to be either a bug in glibc, or my improper specification of a TZ setting, so I am leaving the offending tests listed in virtimetest.c, but disabling them for now. --- See https://www.redhat.com/archives/libvir-list/2014-May/msg00898.html for my earlier comments on this problem. src/util/virtime.c | 3 +++ tests/virtimetest.c | 24 +++++++++++++++++------- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/util/virtime.c b/src/util/virtime.c index 3a56400..c69dff1 100644 --- a/src/util/virtime.c +++ b/src/util/virtime.c @@ -377,6 +377,9 @@ virTimeLocalOffsetFromUTC(long *offset) return -1; } + /* tell mktime to figure out itself whether or not DST is in effect */ + gmtimeinfo.tm_isdst = -1; + /* mktime() also obeys current timezone rules */ if ((utc = mktime(&gmtimeinfo)) == (time_t)-1) { virReportSystemError(errno, "%s", diff --git a/tests/virtimetest.c b/tests/virtimetest.c index bf27682..35551ea 100644 --- a/tests/virtimetest.c +++ b/tests/virtimetest.c @@ -161,20 +161,30 @@ mymain(void) TEST_LOCALOFFSET("VIR00:30", -30 * 60); TEST_LOCALOFFSET("VIR01:30", -90 * 60); + TEST_LOCALOFFSET("VIR05:00", (-5 * 60) * 60); TEST_LOCALOFFSET("UTC", 0); TEST_LOCALOFFSET("VIR-00:30", 30 * 60); TEST_LOCALOFFSET("VIR-01:30", 90 * 60); -#if __TEST_DST + /* test DST processing with timezones that always * have DST in effect; what's more, cover a zone with * with an unusual DST different than a usual one hour */ - /* NB: These tests fail at certain times of the day, so - * must be disabled until we figure out why - */ - TEST_LOCALOFFSET("VIR-00:30VID,0,365", 90 * 60); - TEST_LOCALOFFSET("VIR-02:30VID,0,365", 210 * 60); - TEST_LOCALOFFSET("VIR-02:30VID-04:30,0,365", 270 * 60); + TEST_LOCALOFFSET("VIR-00:30VID,0/00:00:00,366/23:59:59", + ((1 * 60) + 30) * 60); + TEST_LOCALOFFSET("VIR-02:30VID,0/00:00:00,366/23:59:59", + ((3 * 60) + 30) * 60); + TEST_LOCALOFFSET("VIR-02:30VID-04:30,0/00:00:00,366/23:59:59", + ((4 * 60) + 30) * 60); + TEST_LOCALOFFSET("VIR-12:00VID-13:00,0/00:00:00,366/23:59:59", + ((13 * 60) + 0) * 60); +#ifdef __BROKEN_DST_TESTS + TEST_LOCALOFFSET("VIR02:45VID00:45,0/00:00:00,366/23:59:59", + -45 * 60); + TEST_LOCALOFFSET("VIR05:00VID04:00,0/00:00:00,366/23:59:59", + ((-4 * 60) + 0) * 60); + TEST_LOCALOFFSET("VIR11:00VID10:00,0/00:00:00,366/23:59:59", + ((-10 * 60) + 0) * 60); #endif return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; -- 1.9.3
On 05/27/2014 08:11 AM, Laine Stump wrote:
The original version of virTimeLocalOffsetFromUTC() (commit 1cddaea7aeca441b733c31990a3f139dd2d346f6) would fail for certain times of the day if daylight savings time was active. This could most easily be seen by uncommenting the TEST_LOCALOFFSET() cases that invlude a
s/invlude/include/
DST setting.
After a lot of experimenting, I found that the way to solve it in almost all test cases is to set tm_isdst = -1 in the stuct tm prior to calling mktime(). Once this is done, the correct offset is returned for all test cases at all times except the two hours just after 00:00:00 Jan 1 UTC - during that time, any timezone that is *behind* UTC, and that is supposed to always be in DST will not have DST accounted for in its offset. I still do not know the source of this problem, but it appears to be either a bug in glibc, or my improper specification of a TZ setting, so I am leaving the offending tests listed in virtimetest.c, but disabling them for now.
Per POSIX, the default time for swapping in or out of DST is at 2 am unless $TZ says otherwise. So in your testing, VIR05:00VID04:00,0/00:00:00,366/23:59:59 has only a 1 minute window where DST was not active, while VIR05:00VID04:00,0,366 has a 24 hour window that overlaps two days.
---
See https://www.redhat.com/archives/libvir-list/2014-May/msg00898.html for my earlier comments on this problem.
src/util/virtime.c | 3 +++ tests/virtimetest.c | 24 +++++++++++++++++------- 2 files changed, 20 insertions(+), 7 deletions(-)
+#ifdef __BROKEN_DST_TESTS + TEST_LOCALOFFSET("VIR02:45VID00:45,0/00:00:00,366/23:59:59", + -45 * 60); + TEST_LOCALOFFSET("VIR05:00VID04:00,0/00:00:00,366/23:59:59", + ((-4 * 60) + 0) * 60); + TEST_LOCALOFFSET("VIR11:00VID10:00,0/00:00:00,366/23:59:59", + ((-10 * 60) + 0) * 60); #endif
So is the point that these tests are only broken as a year rolls over, while the code itself is robust, and all because we can't _quite_ force our fake timezone to have daylight savings where we want it? Might it be worth doing a simple filter (if time()/localtime_r() says today is Jan 1 or Dec 31, skip these tests, otherwise run them), so that we are at least testing the code 363 days a year? At any rate, the change to virtime.c is essential (you convinced me of that in the other thread), and this counts as a bug fix; it also passes the testsuite. So regardless of whether you keep the #ifdef or convert it to a time-dependent filter, ACK to getting this in before the release. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/27/2014 05:30 PM, Eric Blake wrote:
On 05/27/2014 08:11 AM, Laine Stump wrote:
The original version of virTimeLocalOffsetFromUTC() (commit 1cddaea7aeca441b733c31990a3f139dd2d346f6) would fail for certain times of the day if daylight savings time was active. This could most easily be seen by uncommenting the TEST_LOCALOFFSET() cases that invlude a s/invlude/include/
DST setting.
After a lot of experimenting, I found that the way to solve it in almost all test cases is to set tm_isdst = -1 in the stuct tm prior to calling mktime(). Once this is done, the correct offset is returned for all test cases at all times except the two hours just after 00:00:00 Jan 1 UTC - during that time, any timezone that is *behind* UTC, and that is supposed to always be in DST will not have DST accounted for in its offset. I still do not know the source of this problem, but it appears to be either a bug in glibc, or my improper specification of a TZ setting, so I am leaving the offending tests listed in virtimetest.c, but disabling them for now. Per POSIX, the default time for swapping in or out of DST is at 2 am unless $TZ says otherwise. So in your testing,
VIR05:00VID04:00,0/00:00:00,366/23:59:59
has only a 1 minute window where DST was not active, while
VIR05:00VID04:00,0,366
has a 24 hour window that overlaps two days.
except that "366" is Dec 31 + 1 (Dec 32? or Jan 1 of the next year?) on a leap year, or Dec 31 + 2 on a non-leap year.. The odd part is that if you set such a TZ, then do "date --set ..." for all dates/times around Dec 31 - Jan 1, the output of the new date will *always* show that DST is set (e.g. "VID") (Oh, and also that the failure seems to be from midnight to 2AM *UTC* regardless of the offset of the TZ (even though the DST changes are supposed to happen at 00:00:00 of localtime)).
---
See https://www.redhat.com/archives/libvir-list/2014-May/msg00898.html for my earlier comments on this problem.
src/util/virtime.c | 3 +++ tests/virtimetest.c | 24 +++++++++++++++++------- 2 files changed, 20 insertions(+), 7 deletions(-)
+#ifdef __BROKEN_DST_TESTS + TEST_LOCALOFFSET("VIR02:45VID00:45,0/00:00:00,366/23:59:59", + -45 * 60); + TEST_LOCALOFFSET("VIR05:00VID04:00,0/00:00:00,366/23:59:59", + ((-4 * 60) + 0) * 60); + TEST_LOCALOFFSET("VIR11:00VID10:00,0/00:00:00,366/23:59:59", + ((-10 * 60) + 0) * 60); #endif So is the point that these tests are only broken as a year rolls over, while the code itself is robust, and all because we can't _quite_ force our fake timezone to have daylight savings where we want it?
Yes, after a lot of trials (both with this code and other separate test programs and shell scripts) and thought, I believe that is the case.
Might it be worth doing a simple filter (if time()/localtime_r() says today is Jan 1 or Dec 31, skip these tests, otherwise run them), so that we are at least testing the code 363 days a year?
I like that idea!
At any rate, the change to virtime.c is essential (you convinced me of that in the other thread), and this counts as a bug fix; it also passes the testsuite. So regardless of whether you keep the #ifdef or convert it to a time-dependent filter, ACK to getting this in before the release.
Okay, I will replace the #ifdef with the check you mention above, and send an inter-diff for ACK before pushing. Thanks for all the time and thought you've put into this. It has been a real eye opening experience :-P
participants (2)
-
Eric Blake -
Laine Stump