Re: [libvirt] [PATCH 4/5] qemuDomainResumeFlags: Introduce VIR_DOMAIN_RESUME_SYNC_TIME

On 11/05/14 13:35, Michal Privoznik wrote: > This flag can be used to sync the domain's time right after > domain CPUs are started. It's basically backing call of two > subsequent APIs into one: > > 1) virDomainResume(dom) > 2) virDomainSetTime(dom, 0, 0, VIR_DOMAIN_TIME_SYNC) > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > include/libvirt/libvirt-domain.h | 4 ++++ > src/qemu/qemu_driver.c | 7 ++++++- > 2 files changed, 10 insertions(+), 1 deletion(-) >

> + > + if (flags & VIR_DOMAIN_RESUME_SYNC_TIME && > + qemuDomainSetTimeImpl(driver, vm, 0, 0, true) < 0) > + goto endjob; > + This is a bit problematic. As you are compounding two operations into one, if syncing of the time fails, you've already resumed the guest. Now with this code you would return failure of the entire API call, which would have the caller figure out what happened. Said this, I'm not against this idea but: 1) You need to make it painfully obvious in the error message that the guest was resumed at this point. 2) You also need to make it obvious in the function docs that this might happen and also mention that to have better control the user is better off with calling the time sync function separately. 3) You should also document that the time sync will not happen exactly at the time the guest was resumed. (Some instructions were executed with the old time setting)