Re: [libvirt] [PATCHv2 4/4] qemu: fix <clock offset='variable' basis='localtime'/>

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