Re: [libvirt] [PATCH v4] util: new function virTimeLocalOffsetFromUTC

And don't forget to update comments if localtime_r is removed :) > -----Original Message----- > From: libvir-list-bounces(a)redhat.com [mailto:libvir-list-bounces@redhat.com] > On Behalf Of Laine Stump > Sent: Tuesday, May 27, 2014 7:46 PM > To: libvir-list(a)redhat.com > Subject: Re: [libvirt] [PATCH v4] util: new function virTimeLocalOffsetFromUTC > > On 05/24/2014 05:21 PM, Eric Blake wrote: > > From: Laine Stump <laine(a)laine.org&gt; > > > > Since there isn't a single libc API to get this value, this patch > > supplies one which gets the value by grabbing current time, then > > converting that into a struct tm with gmtime_r(), then back to a > > time_t using mktime. > > > > The returned value is the difference between UTC and localtime in > > seconds. If localtime is ahead of UTC (east) the offset will be a > > positive number, and if localtime is behind UTC (west) the offset will > > be negative. > > > > This function should be POSIX-compliant, and is threadsafe, but not > > async signal safe. If it was ever necessary to know this value in a > > child process, we could cache it with a one-time init function when > > libvirtd starts, then just supply the cached value, but that > > complexity isn't needed for current usage; that would also have the > > problem that it might not be accurate after a local daylight savings > > boundary. > > > > (If it weren't for DST, we could simply replace this entire function > > with "-timezone"; timezone contains the offset of the current timezone > > (negated from what we want) but doesn't account for DST. And in spite > > of being guaranteed by POSIX, it isn't available on older versions of > > mingw.) > > > > Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; > > --- > > > > After some more playing around, I learned we don't need localtime_r > > at all: time() and mktime() both honor the current timezone, so the > > original and resulting value were always the same. I also figured > > out how to force a timezone with a daylight savings different than > > an hour; the code still works without having to hardcode any guess > > based on tm_isdst. > > Actually this patch doesn't work as well as an initial test might > indicate. When I first reviewed it on Saturday afternoon (UTC+3), it > passed the tests, but when I went back and ran it again in the morning > on Monday, it failed the tests that had DST set. > > In the interest of getting as much testing time as possible on the code > (and with DV's agreement), I disabled the failing tests and pushed the > patches, then began investigating in more detail. > > I ran some custom-written tests based on this patch on a Fedora guest > while modifying the system date and found that at different times of the > day the tests that involved DST failed, yet at some times they failed > (the failure times of different tests were different, and as the clock > ticked forward, TZs with larger offsets would begin to succeed). > > I spent a significant amount of time experimenting and found that the > function as written results in a *lot* of failures over the course of a > year (although there are certain times of certain days when it is > successful for some or all of the tests). > > So I tried setting gmtimeinfo.tm_isdst = -1 prior to calling mktime and > Yay! It seemed to succeed for all tests on all dates within a year. Then > I added a few more tests, and changed my test script to change the > system to a different timezone prior to calling the test - after seeing > a pattern during a couple tests, to save time I changed the test to only > run through the clock on Jan 1, May 31, and Dec 31, but to do that both > with the shell environment set to > > TZ="MST07:00MDT06:00,0/00:00:00,366/23:59:59" > > and to > > TZ="EET-02:00EEDT-03:00,0/00:00:00,366/23:59:59". > > (in recognition of my timezone and Eric's timezone :-) > > I've found that in the both cases only three of the 4 new tests I'd > added will fail - these two new tests: > > 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); > > will fail for: > > Dec 31 18:00-18:59 for MST > Jan 1 from 03:00 - 03:59 for EET > > and this test: > > TEST_LOCALOFFSET("VIR02:45VID00:45,0/00:00:00,366/23:59:59", -45 * > 60); > > fails for > > Dec 31 18:00-19:59 for MST > Jan 1 from 03:00 - 04:59 for EET > > (Note that the output of the date command *always* showed "MDT" or > "EEDT", indicating that the trick of forcing DST for every time of every > day was working properly, at least for the time in the shell). > > The failure in all cases is that the offset returned is the offset that > would be correct if DST hadn't been in effect. Since the method of > setting permanent DST appears to work correctly, and it's happening for > an entire 1-2 hour stretch, I'm guessing that the problem isn't that > time() is called while DST is off and then mktime called while it is on, > but something more insidious. > > My conclusions from all this are: > > 1) *definitely* we need to set gmtimeinfo.tm_isdst = -1 in > virTimeLocalOffsetFromUTC(), > 2) I guess we can add all the net test cases I've added that contain > DST, but we should comment out those that fail on Jan 1 / Dec 31. > > I'll send a patch in about an hour to do that. > > (P.S. if you're interested in the ugly shell script I'm using to flip > through the dates, let me know :-) > > > > > src/libvirt_private.syms | 1 + > > src/util/virtime.c | 45 > +++++++++++++++++++++++++++++++++++++++- > > src/util/virtime.h | 5 +++-- > > tests/virtimetest.c | 53 > ++++++++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 101 insertions(+), 3 deletions(-) > > > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > > index 25dab2d..91f13a4 100644 > > --- a/src/libvirt_private.syms > > +++ b/src/libvirt_private.syms > > @@ -1984,6 +1984,7 @@ virTimeFieldsNow; > > virTimeFieldsNowRaw; > > virTimeFieldsThen; > > virTimeFieldsThenRaw; > > +virTimeLocalOffsetFromUTC; > > virTimeMillisNow; > > virTimeMillisNowRaw; > > virTimeStringNow; > > diff --git a/src/util/virtime.c b/src/util/virtime.c > > index caa4e24..3a56400 100644 > > --- a/src/util/virtime.c > > +++ b/src/util/virtime.c > > @@ -1,7 +1,7 @@ > > /* > > * virtime.c: Time handling functions > > * > > - * Copyright (C) 2006-2012 Red Hat, Inc. > > + * Copyright (C) 2006-2014 Red Hat, Inc. > > * > > * This library is free software; you can redistribute it and/or > > * modify it under the terms of the GNU Lesser General Public > > @@ -344,3 +344,46 @@ char *virTimeStringThen(unsigned long long when) > > > > return ret; > > } > > + > > +/** > > + * virTimeLocalOffsetFromUTC: > > + * > > + * > > + * @offset: pointer to time_t that will be set to the difference > > + * between localtime and UTC in seconds (east of UTC is a > > + * positive number, and west of UTC is a negative number. > > + * > > + * Returns 0 on success, -1 on error with error reported > > + */ > > +int > > +virTimeLocalOffsetFromUTC(long *offset) > > +{ > > + struct tm gmtimeinfo; > > + time_t current, utc; > > + > > + /* time() gives seconds since Epoch in current timezone */ > > + if ((current = time(NULL)) == (time_t)-1) { > > + virReportSystemError(errno, "%s", > > + _("failed to get current system time")); > > + return -1; > > + } > > + > > + /* treat current as if it were in UTC */ > > + if (!gmtime_r(&current, &gmtimeinfo)) { > > + virReportSystemError(errno, "%s", > > + _("gmtime_r failed")); > > + return -1; > > + } > > + > right here is where I added: > > gmtimeinfo.tm_isdst = -1; > > 1, 0, and leaving it alone all caused more failures. > > > + /* mktime() also obeys current timezone rules */ > > + if ((utc = mktime(&gmtimeinfo)) == (time_t)-1) { > > + virReportSystemError(errno, "%s", > > + _("mktime failed")); > > + return -1; > > + } > > + > > + *offset = current - utc; > > + return 0; > > +} > > +#define TEST_LOCALOFFSET(tz, off) \ > > + do { \ > > + testTimeLocalOffsetData data = { \ > > + .zone = tz, \ > > + .offset = off, \ > > + }; \ > > + if (virtTestRun("Test localtime offset for " #tz, \ > > + testTimeLocalOffset, &data) < 0) \ > > + ret = -1; \ > > + } while (0) > > + > > + TEST_LOCALOFFSET("VIR00:30", -30 * 60); > > + TEST_LOCALOFFSET("VIR01:30", -90 * 60); > > + TEST_LOCALOFFSET("UTC", 0); > > + TEST_LOCALOFFSET("VIR-00:30", 30 * 60); > > + TEST_LOCALOFFSET("VIR-01:30", 90 * 60); > > + /* 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 > > + */ > > + 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); > > Here are my new tests: > > > 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-02:30VID-04:30,0/00:00:00,366/23:59:59", ((4 * > 60) + 30) * 60); > * > TEST_LOCALOFFSET("VIR02:45VID00:45,0/00:00:00,366/23:59:59", > -45 * 60); > TEST_LOCALOFFSET("VIR-12:00VID-13:00,0/00:00:00,366/23:59:59",((13 > * > 60) + 0) * 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); > > (those marked with "*" fail on Dec 31 or Jan 1, depending on TZ setting > of the shell that is running virtimetest) > > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list