[libvirt] [PATCHv2 1/4] util: new function virTimeLocalOffsetFromUTC

[Laine Stump]

On 05/22/2014 10:03 PM, Eric Blake wrote:
> On 05/22/2014 05:07 AM, Laine Stump wrote:
>> Since there isn't a single libc API to get this value, this patch
>> supplies one which gets the value by grabbing current UTC, then
>> converting that into a struct tm with localtime_r(), then back to a
>> time_t using mktime; it again does the same operation, but using
>> gmtime_r() instead (for UTC). It then subtracts utc time from the
>> localtime, and finally adjusts if dst is set in the localtime timeinfo
>> (because for some reason mktime doesn't take that into account).
>>
>> 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.
>> ---
>> +
>> +/**
>> + * virTimeLocalOffsetFromUTC:
>> + *
>> + * This function is threadsafe, but is *not* async signal safe (due to
>> + * localtime_r()).
>> + *
>> + * @offset: pointer to time_t that will difference between localtime
> s/will/will be set to the/

fixed

>
>> + *          and UTC in seconds.
> Should you also mention whether positive is east of UTC vs. west of UTC?

Done.


>  time_t is not necessarily a signed type, but even if it is unsigned,
> you can treat it as a 2s-complement negative value.  Maybe this function
> should take 'long *offset' instead of 'time_t *offset', to match the
> POSIX 'timezone' variable?
>
>> + *
>> + * Returns 0 on success, -1 on error with error reported
>> + */
>> +int
>> +virTimeLocalOffsetFromUTC(time_t *offset)
>> +{
>> +    struct tm gmtimeinfo, localtimeinfo;
>> +    time_t current, utc, local;
>> +
>> +    if ((current = time(NULL)) < 0) {
> time_t is allowed to be an unsigned type.  The only portable way to
> check for failure is:
>
> if ((current = time(NULL)) == (time_t)-1) {

Done.

>
>> +        virReportSystemError(errno, "%s",
>> +                             _("failed to get current system time"));
>> +        return -1;
>> +    }
>> +
>> +    localtime_r(&current, &localtimeinfo);
>> +    gmtime_r(&current, &gmtimeinfo);
> localtime_r() and gmtime_r() can fail (returning NULL, and leaving
> unspecified contents in the 2nd argument) - although arguably the
> failure can only occur due to EOVERFLOW at the year 2038 boundary.  So
> ignoring the error is not necessarily fatal, although it might be worth
> a command and/or ignore_value() to state why we are ignoring failure.

Nah, I just changed it to check both of those for error.

>
>> +
>> +    if ((local = mktime(&localtimeinfo)) < 0 ||
>> +        (utc = mktime(&gmtimeinfo)) < 0) {
> Two more comparisons that must be made against ==(time_t)-1, rather than
> <0, due to time_t possibly being unsigned.  What's more, mktime() will
> fail only for EOVERFLOW; but if that's the case, then the earlier
> localtime_r() or even time() calls would have failed.  It seems
> inconsistent to check here but not earlier.

Okay, I've fixed the comparisons.

>
>> +        virReportSystemError(errno, "%s",
>> +                             _("mktime failed"));
>> +        return -1;
>> +    }
>> +
>> +    *offset = local - utc;
>> +    if (localtimeinfo.tm_isdst)
>> +        *offset += 3600;
> Technically, POSIX allows for a daylight savings that is different than
> a mere 3600-second gap.  But I'm not sure how you would be able to
> figure that out from struct tm.
>
> It would be a LOT simpler to just do:
>
> #include <time.h>
>
> tzset();
> *offset = timezone;
>
> except that some older builds of mingw lack the extern variable
> timezone.  Or maybe even do a configure check, for
> AC_CHECK_DECLS([timezone]) (untested, just throwing out the idea), and
> having #if HAVE_DECL_TIMEZONE with the short code and the #else clause
> using this dance as the fallback for mingw?  Or even just ditch older
> mingw?  I see this in Fedora 20's cross-packages for mingw:

I'm so sick of this topic right now that I'd prefer leaving as is,
although that is tempting.

Let me think about it for the next couple hours and get back to you.

>
> /usr/i686-w64-mingw32/sys-root/mingw/include/time.h:  __MINGW_IMPORT
> long _timezone;
> /usr/i686-w64-mingw32/sys-root/mingw/include/time.h:  _CRTIMP extern
> long timezone;
>
> but have no idea how accurate they are.
>
>> @@ -119,6 +148,21 @@ mymain(void)
>>  
>>      TEST_FIELDS(2147483648000ull, 2038,  1, 19,  3, 14,  8);
>>  
>> +#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("UTC", 0);
>> +    TEST_LOCALOFFSET("VIR-00:30", 30 * 60);
> This looks accurate, but as you say, it doesn't do any testing of
> daylight savings to know if we're getting that part right.
>

I've added in tests as discussed in other mails.