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

[Eric Blake]

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/

> + *          and UTC in seconds.

Should you also mention whether positive is east of UTC vs. west of UTC?
 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) {

> +        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.

> +
> +    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.

> +        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:

/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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140522/08e12b75/attachment-0001.sig>