[libvirt] [PATCHv2 4/4] qemu: fix <clock offset='variable' basis='localtime'/>
Eric Blake
eblake at redhat.com
Thu May 22 19:40:22 UTC 2014
On 05/22/2014 05:07 AM, Laine Stump wrote:
> 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.
>
So basically, we are arguing that basis='localtime' should be treated as
syntactic sugar which we rewrite to a more convenient form at the time
we start the guest, and not as a permanent basis that we attempt to
migrate. I can live with that.
> 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).
Okay, so this patch is cementing some of the arguments I made in
response to 3/4 (before I had read this patch).
>
> 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
s/an/to an/
> + * 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).
including migration to file, across a single host that changes localtime
RTC across daylight savings between when the file was saved and when it
gets loaded :)
> + */
> + 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);
Seems to make sense. But I'd like feedback from qemu on 3/4 before
giving outright ack to this patch.
> +
> /* 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
>
--
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/b2078bda/attachment-0001.sig>
More information about the libvir-list
mailing list