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. --- Change from V1: add test cases with TZ set to different values (if someone knows how to force DST on/off, I would gladly add some test cases for this as well). src/libvirt_private.syms | 1 + src/util/virtime.c | 41 ++++++++++++++++++++++++++++++++++++++++- src/util/virtime.h | 5 +++-- tests/virtimetest.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 3 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c3332c9..cb635cd 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..c487eba 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,42 @@ 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 difference between localtime + * and UTC in seconds. + * + * 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) { + virReportSystemError(errno, "%s", + _("failed to get current system time")); + return -1; + } + + localtime_r(&current, &localtimeinfo); + gmtime_r(&current, &gmtimeinfo); + + if ((local = mktime(&localtimeinfo)) < 0 || + (utc = mktime(&gmtimeinfo)) < 0) { + virReportSystemError(errno, "%s", + _("mktime failed")); + return -1; + } + + *offset = local - utc; + if (localtimeinfo.tm_isdst) + *offset += 3600; + return 0; +} diff --git a/src/util/virtime.h b/src/util/virtime.h index a5bd652..cda9707 100644 --- a/src/util/virtime.h +++ b/src/util/virtime.h @@ -1,7 +1,7 @@ /* * virtime.h: Time handling functions * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2011, 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 @@ -50,7 +50,6 @@ int virTimeStringNowRaw(char *buf) int virTimeStringThenRaw(unsigned long long when, char *buf) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; - /* These APIs are *not* async signal safe and return -1, * raising a libvirt error on failure */ @@ -63,5 +62,7 @@ int virTimeFieldsThen(unsigned long long when, struct tm *fields) char *virTimeStringNow(void); char *virTimeStringThen(unsigned long long when); +int virTimeLocalOffsetFromUTC(time_t *offset) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; #endif diff --git a/tests/virtimetest.c b/tests/virtimetest.c index 73a3c3e..d1a409f 100644 --- a/tests/virtimetest.c +++ b/tests/virtimetest.c @@ -72,6 +72,35 @@ static int testTimeFields(const void *args) } +typedef struct { + const char *zone; + time_t offset; +} testTimeLocalOffsetData; + +static int +testTimeLocalOffset(const void *args) +{ + const testTimeLocalOffsetData *data = args; + time_t actual; + + if (setenv("TZ", data->zone, 1) < 0) { + perror("setenv"); + return -1; + } + tzset(); + + if (virTimeLocalOffsetFromUTC(&actual) < 0) { + return -1; + } + + if (data->offset != actual) { + VIR_DEBUG("Expect Offset %d got %d\n", + (int) data->offset, (int) actual); + return -1; + } + return 0; +} + static int mymain(void) { @@ -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); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.9.0

On 05/22/2014 10:03 PM, Eric Blake wrote: fixed Done. Done. Nah, I just changed it to check both of those for error. Okay, I've fixed the comparisons. 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. I've added in tests as discussed in other mails.

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 daylight savings time (DST) is set in the localtime timeinfo (because for some reason mktime doesn't take that into account). 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. Note that it is theoretically possible to set a DST offset that is not exactly one hour. This function doesn't handle that (is that actually in use anywhere?) (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.) --- Change from V2: * added test cases that force DST to be active so that the "tm_isdst" logic can be properly tested. * addressed Eric's review points. Note that I tried out using the global "timezone", but it doesn't adjust for DST, so I decided to keep a single implementation of the function. src/libvirt_private.syms | 1 + src/util/virtime.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++- src/util/virtime.h | 5 +++-- tests/virtimetest.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 3 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c3332c9..cb635cd 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..79b7bd5 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,51 @@ 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, localtimeinfo; + time_t current, utc, local; + + if ((current = time(NULL)) == (time_t)-1) { + virReportSystemError(errno, "%s", + _("failed to get current system time")); + return -1; + } + + if (!localtime_r(&current, &localtimeinfo)) { + virReportSystemError(errno, "%s", + _("localtime_r failed")); + return -1; + } + if (!gmtime_r(&current, &gmtimeinfo)) { + virReportSystemError(errno, "%s", + _("gmtime_r failed")); + return -1; + } + + if ((local = mktime(&localtimeinfo)) == (time_t)-1 || + (utc = mktime(&gmtimeinfo)) == (time_t)-1) { + virReportSystemError(errno, "%s", + _("mktime failed")); + return -1; + } + + *offset = local - utc; + if (localtimeinfo.tm_isdst) + *offset += 3600; + return 0; +} diff --git a/src/util/virtime.h b/src/util/virtime.h index a5bd652..25332db 100644 --- a/src/util/virtime.h +++ b/src/util/virtime.h @@ -1,7 +1,7 @@ /* * virtime.h: Time handling functions * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2011, 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 @@ -50,7 +50,6 @@ int virTimeStringNowRaw(char *buf) int virTimeStringThenRaw(unsigned long long when, char *buf) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; - /* These APIs are *not* async signal safe and return -1, * raising a libvirt error on failure */ @@ -63,5 +62,7 @@ int virTimeFieldsThen(unsigned long long when, struct tm *fields) char *virTimeStringNow(void); char *virTimeStringThen(unsigned long long when); +int virTimeLocalOffsetFromUTC(long *offset) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; #endif diff --git a/tests/virtimetest.c b/tests/virtimetest.c index 73a3c3e..a14dcb9 100644 --- a/tests/virtimetest.c +++ b/tests/virtimetest.c @@ -72,6 +72,35 @@ static int testTimeFields(const void *args) } +typedef struct { + const char *zone; + long offset; +} testTimeLocalOffsetData; + +static int +testTimeLocalOffset(const void *args) +{ + const testTimeLocalOffsetData *data = args; + long actual; + + if (setenv("TZ", data->zone, 1) < 0) { + perror("setenv"); + return -1; + } + tzset(); + + if (virTimeLocalOffsetFromUTC(&actual) < 0) { + return -1; + } + + if (data->offset != actual) { + VIR_DEBUG("Expect Offset %ld got %ld\n", + data->offset, actual); + return -1; + } + return 0; +} + static int mymain(void) { @@ -119,6 +148,28 @@ 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("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 + */ + TEST_LOCALOFFSET("VIR-00:30VID,0,365", 90 * 60); + TEST_LOCALOFFSET("VIR-02:30VID,0,365", 210 * 60); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.9.0

On 05/24/2014 05:21 PM, Eric Blake wrote: I could have sworn I tried exactly the same thing back in the very beginning and ended up with the offset always being 0. But since all the tests pass, I'm now guessing that I must have mistakenly done only localtime_r() then did a mktime on it (which would give back the original time_t). Your version is simpler and works in more cases, so I would ACK if I could, but as I'm the original author I don't think my ACK can count, can it? Or does your modification of my original patch count as an ACK on the parts it didn't change? In short, do we need ACK from a 3rd person to push this?

And don't forget to update comments if localtime_r is removed :) > -----Original Message----- > From: libvir-list-bounces(a)redhat.com [mailto:libvir-list-bounces@redhat.com] > On Behalf Of Laine Stump > Sent: Tuesday, May 27, 2014 7:46 PM > To: libvir-list(a)redhat.com > Subject: Re: [libvirt] [PATCH v4] util: new function virTimeLocalOffsetFromUTC > > On 05/24/2014 05:21 PM, Eric Blake wrote: > > From: Laine Stump <laine(a)laine.org&gt; > > > > 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(a)redhat.com&gt; > > --- > > > > 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: > > + * > > + * > > + * @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(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list

This reverts commit e31b5cf393857a6ca78d148c19393e28dfb39de1. This commit attempted to work around a bug in the offset value reported by qemu's RTC_CHANGE event in the case that a variable base date was given on the qemu commandline. The patch mixed up the math involved in arriving at the corrected offset to report, and in the process added an unnecessary private attribute to the clock element. Since that element is private/internal and not used by anyone else, it makes sense to simplify things by removing it. --- Unchanged from V2. This is the direct result of: git revert e31b5cf393857a6ca78d148c19393e28dfb39de1 src/conf/domain_conf.c | 27 ++++----------------------- src/conf/domain_conf.h | 5 ----- src/qemu/qemu_command.c | 3 --- src/qemu/qemu_process.c | 13 ------------- 4 files changed, 4 insertions(+), 44 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 055b979..a7863c3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -99,7 +99,6 @@ typedef enum { VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES = (1<<18), VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM = (1<<19), VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT = (1<<20), - VIR_DOMAIN_XML_INTERNAL_BASEDATE = (1 << 21), } virDomainXMLInternalFlags; VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, @@ -12026,16 +12025,6 @@ virDomainDefParseXML(xmlDocPtr xml, break; } - if (def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE && - flags & VIR_DOMAIN_XML_INTERNAL_BASEDATE) { - if (virXPathULongLong("number(./clock/@basedate)", ctxt, - &def->clock.data.variable.basedate) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("invalid basedate")); - goto error; - } - } - if ((n = virXPathNodeSet("./clock/timer", ctxt, &nodes)) < 0) goto error; @@ -17097,8 +17086,7 @@ virDomainResourceDefFormat(virBufferPtr buf, verify(((VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | - VIR_DOMAIN_XML_INTERNAL_BASEDATE) + VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES) & DUMPXML_FLAGS) == 0); /* This internal version can accept VIR_DOMAIN_XML_INTERNAL_*, @@ -17120,8 +17108,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virCheckFlags(DUMPXML_FLAGS | VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | - VIR_DOMAIN_XML_INTERNAL_BASEDATE, + VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES, -1); if (!(type = virDomainVirtTypeToString(def->virtType))) { @@ -17655,10 +17642,6 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, " adjustment='%lld' basis='%s'", def->clock.data.variable.adjustment, virDomainClockBasisTypeToString(def->clock.data.variable.basis)); - - if (flags & VIR_DOMAIN_XML_INTERNAL_BASEDATE) - virBufferAsprintf(buf, " basedate='%llu'", - def->clock.data.variable.basedate); break; case VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE: virBufferEscapeString(buf, " timezone='%s'", def->clock.data.timezone); @@ -18089,8 +18072,7 @@ virDomainSaveStatus(virDomainXMLOptionPtr xmlopt, unsigned int flags = (VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | - VIR_DOMAIN_XML_INTERNAL_BASEDATE); + VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES); int ret = -1; char *xml; @@ -18178,8 +18160,7 @@ virDomainObjListLoadStatus(virDomainObjListPtr doms, if (!(obj = virDomainObjParseFile(statusFile, caps, xmlopt, expectedVirtTypes, VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | - VIR_DOMAIN_XML_INTERNAL_BASEDATE))) + VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES))) goto error; virUUIDFormat(obj->def->uuid, uuidstr); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index bde303c..94285e3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1727,11 +1727,6 @@ struct _virDomainClockDef { struct { long long adjustment; int basis; - - /* Store the base date (-rtc base=$date, in seconds - * since the Epoch) of guest process, internal only - */ - unsigned long long basedate; } variable; /* Timezone name, when diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 193959f..95f747f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5988,9 +5988,6 @@ qemuBuildClockArgStr(virDomainClockDefPtr def) break; } - /* Store the guest's basedate */ - def->data.variable.basedate = now; - virBufferAsprintf(&buf, "base=%d-%02d-%02dT%02d:%02d:%02d", nowbits.tm_year + 1900, nowbits.tm_mon + 1, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a83780f..6dcb737 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -843,19 +843,6 @@ qemuProcessHandleRTCChange(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virObjectLock(vm); - - /* QEMU's RTC_CHANGE event returns the offset from the specified - * date instead of the host UTC if a specific date is provided - * (-rtc base=$date). We need to convert it to be offset from - * host UTC. - */ - if (vm->def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE) { - time_t now = time(NULL); - - offset += vm->def->clock.data.variable.basedate - - (unsigned long long)now; - } - event = virDomainEventRTCChangeNewFromObj(vm, offset); if (vm->def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE) -- 1.9.0

commit e31b5cf393857 attempted to fix libvirt's VIR_DOMAIN_EVENT_ID_RTC_CHANGE, which is documentated to always provide the new offset of the domain's real time clock from UTC. The problem was that, in the case that qemu is provided with an "-rtc base=x" where x is an absolute time (rather than "utc" or "localtime"), the offset sent by qemu's RTC_CHANGE event is *not* the new offset from UTC, but rather is the sum of all changes to the domain's RTC since it was started with base=x. So, despite what was said in commit e31b5cf393857, if we assume that the original value stored in "adjustment" was the offset from UTC at the time the domain was started, we can always determine the current offset from UTC by simply adding the most recent (i.e. current) offset from qemu to that original adjustment. This patch accomplishes that by storing the initial adjustment in the domain's status as "adjustment0". Each time a new RTC_CHANGE event is received from qemu, we simply add adjustment0 to the value sent by qemu, store that as the new adjustment, and forward that value on to any event handler. This patch (*not* e31b5cf393857, which should be reverted prior to applying this patch) fixes: https://bugzilla.redhat.com/show_bug.cgi?id=964177 (for the case where basis='utc'. It does not fix basis='localtime') --- Changes from V1: remove all attempts to fix basis='localtime' in favor of fixing it in a simpler and better manner in a separate patch. src/conf/domain_conf.c | 21 +++++++++++++++++---- src/conf/domain_conf.h | 7 +++++++ src/qemu/qemu_command.c | 9 +++++++++ src/qemu/qemu_process.c | 27 ++++++++++++++++++++++----- 4 files changed, 55 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a7863c3..cc33b86 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -99,6 +99,7 @@ typedef enum { VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES = (1<<18), VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM = (1<<19), VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT = (1<<20), + VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST = (1<<21), } virDomainXMLInternalFlags; VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, @@ -12002,6 +12003,9 @@ virDomainDefParseXML(xmlDocPtr xml, if (virXPathLongLong("number(./clock/@adjustment)", ctxt, &def->clock.data.variable.adjustment) < 0) def->clock.data.variable.adjustment = 0; + if (virXPathLongLong("number(./clock/@adjustment0)", ctxt, + &def->clock.data.variable.adjustment0) < 0) + def->clock.data.variable.adjustment0 = 0; tmp = virXPathString("string(./clock/@basis)", ctxt); if (tmp) { if ((def->clock.data.variable.basis = virDomainClockBasisTypeFromString(tmp)) < 0) { @@ -17086,7 +17090,8 @@ virDomainResourceDefFormat(virBufferPtr buf, verify(((VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES) + VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | + VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST) & DUMPXML_FLAGS) == 0); /* This internal version can accept VIR_DOMAIN_XML_INTERNAL_*, @@ -17108,7 +17113,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, virCheckFlags(DUMPXML_FLAGS | VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES, + VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | + VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST, -1); if (!(type = virDomainVirtTypeToString(def->virtType))) { @@ -17642,6 +17648,11 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, " adjustment='%lld' basis='%s'", def->clock.data.variable.adjustment, virDomainClockBasisTypeToString(def->clock.data.variable.basis)); + if (flags & VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST) { + if (def->clock.data.variable.adjustment0) + virBufferAsprintf(buf, " adjustment0='%lld'", + def->clock.data.variable.adjustment0); + } break; case VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE: virBufferEscapeString(buf, " timezone='%s'", def->clock.data.timezone); @@ -18072,7 +18083,8 @@ virDomainSaveStatus(virDomainXMLOptionPtr xmlopt, unsigned int flags = (VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES); + VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | + VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST); int ret = -1; char *xml; @@ -18160,7 +18172,8 @@ virDomainObjListLoadStatus(virDomainObjListPtr doms, if (!(obj = virDomainObjParseFile(statusFile, caps, xmlopt, expectedVirtTypes, VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES))) + VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | + VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST))) goto error; virUUIDFormat(obj->def->uuid, uuidstr); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 94285e3..8f17c74 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1727,6 +1727,13 @@ struct _virDomainClockDef { struct { long long adjustment; int basis; + + /* domain start-time adjustment. This is a + * private/internal read-only value that only exists when + * a domain is running, and only if the clock + * offset='variable' + */ + long long adjustment0; } variable; /* Timezone name, when diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 95f747f..4998bca 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -36,6 +36,7 @@ #include "virfile.h" #include "virnetdev.h" #include "virstring.h" +#include "virtime.h" #include "viruuid.h" #include "c-ctype.h" #include "domain_nwfilter.h" @@ -5988,6 +5989,14 @@ qemuBuildClockArgStr(virDomainClockDefPtr def) break; } + /* when an RTC_CHANGE event is received from qemu, we need to + * have the adjustment used at domain start time available to + * compute the new offset from UTC. As this new value is + * itself stored in def->data.variable.adjustment, we need to + * save a copy of it now. + */ + def->data.variable.adjustment0 = def->data.variable.adjustment; + virBufferAsprintf(&buf, "base=%d-%02d-%02dT%02d:%02d:%02d", nowbits.tm_year + 1900, nowbits.tm_mon + 1, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6dcb737..fb90ac9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -831,7 +831,6 @@ qemuProcessHandleResume(qemuMonitorPtr mon ATTRIBUTE_UNUSED, return 0; } - static int qemuProcessHandleRTCChange(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virDomainObjPtr vm, @@ -843,13 +842,31 @@ qemuProcessHandleRTCChange(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virObjectLock(vm); - event = virDomainEventRTCChangeNewFromObj(vm, offset); - if (vm->def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE) + if (vm->def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE) { + /* when a basedate is manually given on the qemu commandline + * rather than simply "-rtc base=utc", the offset sent by qemu + * in this event is *not* the new offset from UTC, but is + * instead the new offset from the *original basedate* + + * uptime. For example, if the original offset was 3600 and + * the guest clock has been advanced by 10 seconds, qemu will + * send "10" in the event - this means that the new offset + * from UTC is 3610, *not* 10. If the guest clock is advanced + * by another 10 seconds, qemu will now send "20" - i.e. each + * event is the sum of the most recent change and all previous + * changes since the domain was started. Fortunately, we have + * saved the initial offset in "adjustment0", so to arrive at + * the proper new "adjustment", we just add the most recent + * offset to adjustment0. + */ + offset += vm->def->clock.data.variable.adjustment0; vm->def->clock.data.variable.adjustment = offset; - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - VIR_WARN("unable to save domain status with RTC change"); + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + VIR_WARN("unable to save domain status with RTC change"); + } + + event = virDomainEventRTCChangeNewFromObj(vm, offset); virObjectUnlock(vm); -- 1.9.0

On 05/22/2014 05:07 AM, Laine Stump wrote: [I've re-read this thread, trying to determine whether this patch is good enough to apply as-is] I agree that 'adjustment' is a runtime-only value - it only exists as long as a qemu guest is running; I also agree that the offset from UTC requires knowing both 'adjustment0' (the command-line offset) and 'adjustment' (the current qemu state, whether learned by RTC Change event, or by a new [yet-to-be-written] QMP query command). Okay, I'm convinced that this guarantees that the right value is exposed in the event handler, so this patch is a strict improvement. I still think we have one (or more) libvirt bugs (basically, 'adjustment0' is the persistent domain's definition of what the next boot of the guest will start with, and unless the user manually changes that value, I think libvirt should automatically be adjusting the persistent 'adjustment0' to the final value of the run-time when the guest shuts down; I also think VDSM needs a way to query the UTC offset that was last used by a transient domain even after the domain has shut down) - but those can (and should) be separate patches. In addition to the commit typo found earlier, s/clock/clock uses/ Spurious line deletion. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

For a clock element as above, libvirt simply converts current system time with localtime_r(), then starts qemu with a time string that doesn't contain any timezone information. So, from qemu's point of view, the -rtc string it gets for: <clock offset='variable' basis='utc' adjustment='10800'/> is identical to the -rtc string it gets for: <clock offset='variable' basis='localtime' adjustment='0'/> (assuming the host is in a timezone that is 10800 seconds ahead of UTC, as is the case on the machine where this message is being written). Since the commandlines are identical, qemu will behave identically after this point in either case. There are two problems in the case of basis='localtime' though: Problem 1) If the guest modifies its RTC, for example to add 20 seconds, the RTC_CHANGE event from qemu will then contain offset:20 in both cases. But libvirt will have saved the original adjustment into adjustment0, and will add that value onto the offset in the event. This means that in the case of basis=;utc', it will properly emit an event with offset:10820, but in the case of basis='localtime' the event will contain offset:20, which is *not* the new offset of the RTC from UTC (as the event it documented to provide). Problem 2) If the guest is migrated to another host that is in a different timezone, or if it is migrated or saved/restored after the DST status has changed from what it was when the guest was originally started, the newly restarted guest will have a different RTC (since it will be based on the new localtime, which could have shifted by several hours). The solution to both of these problems is simple - rather than maintaining the original adjustment value along with "basis='localtime'" in the domain status, when the domain is started we convert the adjustment offset to one relative to UTC, and set the status to "basis='utc'". Thus, whatever the RTC offset was from UTC when it was initially started, that offset will be maintained when migrating across timezones and DST settings, and the RTC_CHANGE events will automatically contain the proper offset (which should by definition always be relative to UTC). This fixes a problem that was implied but not openly stated in: https://bugzilla.redhat.com/show_bug.cgi?id=964177 --- New in V2 src/qemu/qemu_command.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4998bca..28885f2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5976,19 +5976,30 @@ qemuBuildClockArgStr(virDomainClockDefPtr def) time_t now = time(NULL); struct tm nowbits; - switch ((enum virDomainClockBasis) def->data.variable.basis) { - case VIR_DOMAIN_CLOCK_BASIS_UTC: - now += def->data.variable.adjustment; - gmtime_r(&now, &nowbits); - break; - case VIR_DOMAIN_CLOCK_BASIS_LOCALTIME: - now += def->data.variable.adjustment; - localtime_r(&now, &nowbits); - break; - case VIR_DOMAIN_CLOCK_BASIS_LAST: - break; + if (def->data.variable.basis == VIR_DOMAIN_CLOCK_BASIS_LOCALTIME) { + time_t localOffset; + + /* in the case of basis='localtime', rather than trying to + * keep that basis (and associated offset from UTC) in the + * status and deal with adding in the difference each time + * there is an RTC_CHANGE event, it is simpler and less + * error prone to just convert the adjustment an offset + * from UTC right now (and change the status to + * "basis='utc' to reflect this). This eliminates + * potential errors in both RTC_CHANGE events and in + * migration (in the case that the status of DST, or the + * timezone of the destination host, changed relative to + * startup). + */ + if (virTimeLocalOffsetFromUTC(&localOffset) < 0) + goto error; + def->data.variable.adjustment += localOffset; + def->data.variable.basis = VIR_DOMAIN_CLOCK_BASIS_UTC; } + now += def->data.variable.adjustment; + gmtime_r(&now, &nowbits); + /* when an RTC_CHANGE event is received from qemu, we need to * have the adjustment used at domain start time available to * compute the new offset from UTC. As this new value is -- 1.9.0