[libvirt] [PATCH] qemu driver: sync guest time via qemu-guest-agent when domain is resumed

[Eric Blake]

On 02/10/2014 03:10 PM, Marcelo Tosatti wrote:
> 
> Since QEMU commit "kvmclock: clock should count only if vm is running"
> (http://lists.gnu.org/archive/html/qemu-devel/2013-06/msg01225.html), 
> guests have their realtime clock stopped during pause.
> 
> To correct the situation, invoke guest agent to sync time from
> host time.

This needs to be conditional on new API (either a flag to existing API
[except that virDomainResumeFlags is not quite existing yet], or new API
altogether).

We cannot require an API to depend on guest interaction (which an agent
command does) without an explicit flag mentioning that it is okay.  We
may even decide to have conditional ACL checks (allowing some users the
ability to restart a domain, but not to interact with the guest agent).

For that matter, when we proposed adding virDomainResumeFlags as the way
to add in a flag for requesting a sync via this API, Dan suggested it
would be better to add a new API altogether instead of piggybacking on
top of the existing API:

https://www.redhat.com/archives/libvir-list/2014-February/msg00104.html


> +
> +static void
> +qemuAgentSyncGuestTime(virDomainObjPtr vm)
> +{
> +    int ret;
> +    struct timeval tv;
> +    char *result;
> +    qemuDomainObjPrivatePtr priv;
> +    char buf[500];
> +
> +    priv = vm->privateData;
> +
> +    ret = gettimeofday(&tv, NULL);
> +    if (ret) {
> +        virReportSystemError(errno, "%s", _("gettimeofday failure"));
> +        return;
> +    }
> +
> +    memset(buf, 0, sizeof(buf));
> +
> +    sprintf(buf, "{ \"execute\": \"guest-set-time\","
> +                     "\"arguments\":{\"time\":%lld}}\" ",
> +                    tv.tv_usec * 1000 + (tv.tv_sec * 1000000000LL));

Ewww.  This should be using qemuAgentMakeCommand, not open-coding a
string via printf (and even if we DO stick with printf, it should use
virAsprintf, not sprintf).  qemuAgentGuestSync is not the best example
to be copying.

> +
> +    qemuDomainObjEnterAgent(vm);
> +    ret = qemuAgentArbitraryCommand(priv->agent, buf, &result,
> +                                    VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT);

Again, if you use qemUAgentMakeCommand, then this is much nicer with
qemuAgentCommand().  qemuAgentArbitraryCommand should be reserved only
for the public API in libvirt-qemu.so.

> +    qemuDomainObjExitAgent(vm);
> +    if (ret < 0)
> +        VIR_FREE(result);
> +}
> +
>  /*
>   * Precondition: vm must be locked, and a job must be active.
>   * This method will call {Enter,Exit}Monitor
> @@ -2727,6 +2761,7 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm,
>  
>      if (ret == 0) {
>          virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, reason);
> +        qemuAgentSyncGuestTime(vm);

Making this unconditional as part of starting every guest is flat out
wrong.  It MUST be explicit, as not all guests will have an agent
installed to be able to do this, and even for guests that have an agent
installed, interacting with the guest agent may be deemed too much of a
security risk in some setups.

-- 
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/20140210/c85fb592/attachment-0001.sig>