Re: [PATCH v4] util: new function virTimeLocalOffsetFromUTC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx [mailto:libvir-list-bounces@xxxxxxxxxx]
> On Behalf Of Laine Stump
> Sent: Tuesday, May 27, 2014 7:46 PM
> To: libvir-list@xxxxxxxxxx
> Subject: Re:  [PATCH v4] util: new function virTimeLocalOffsetFromUTC
> 
> On 05/24/2014 05:21 PM, Eric Blake wrote:
> > From: Laine Stump <laine@xxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> >
> > 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(&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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]