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

[Eric Blake]

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>