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(¤t, &localtimeinfo);
> + gmtime_r(¤t, &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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list