9:45 a.m.
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(¤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.
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.