[libvirt] [PATCH v4] util: new function virTimeLocalOffsetFromUTC
Wangrui (K)
moon.wangrui at huawei.com
Wed May 28 07:23:49 UTC 2014
And don't forget to update comments if localtime_r is removed :)
> > + * This function is threadsafe, but is *not* async signal safe (due to
> > + * localtime_r()).
> -----Original Message-----
> From: libvir-list-bounces at redhat.com [mailto:libvir-list-bounces at redhat.com]
> On Behalf Of Laine Stump
> Sent: Tuesday, May 27, 2014 7:46 PM
> To: libvir-list at 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 at laine.org>
> >
> > 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 at redhat.com>
> > ---
> >
> > 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:
> > + *
> > + * This function is threadsafe, but is *not* async signal safe (due to
> > + * localtime_r()).
> > + *
> > + * @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(¤t, &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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list