On Mon, Feb 10, 2014 at 03:42:45PM -0700, Eric Blake wrote: > 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 "So there's interesting, and I'm not sure I entirely agree about adding flags here. It seems to me that if the QEMU agent has a "set-time" capability we'd be better off having an explicit API virDomainSetTime(...). The action of resume + set time cannot be atomic so I don't see any point in overloading the "set time" functionality into the resume API call. Just let apps call the set time method if they so desire." Apps desire the guest time to be synchronized to host time, which is the standard 99% of usecases configuration. Ok, i was thinking of adding a new element GuestSyncTimeOnResume to control this, which would default to on (as users expect their guests time to be correct at all times, without additional commands, and rightly so). It seems unreasonable to me that every API user has to keep track of every location which a guest can possibly resume execution from a paused state: this can be hidden inside libvirt. Daniel, all the logic behind the necessity to set-time after guest resume (*) can be hidden inside libvirt. (*) all instances of guest resume: upon migration continuation, upon ENOSPACE pause continuation, all of them. Don't see the need to force the knowledge and maintenance of this state on libvirt users. Are you OK with the new knob, default on, to control sync time on guest resume, then? > > + > > +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. Ok, i'll use that. > > + > > + 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 OK. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list