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

[Peter Krempa]

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 at redhat.com>
> ---
>  include/libvirt/libvirt-domain.h | 4 ++++
>  src/qemu/qemu_driver.c           | 7 ++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 1795dd3..8d763c7 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -867,6 +867,10 @@ int                     virDomainFree           (virDomainPtr domain);
>  /*
>   * Domain suspend/resume
>   */
> +typedef enum {
> +    VIR_DOMAIN_RESUME_SYNC_TIME = 1 << 0,   /* on resume sync domain time from its RTC */
> +} virDomainResumeFlagsValues;
> +
>  int                     virDomainSuspend        (virDomainPtr domain);
>  int                     virDomainResume         (virDomainPtr domain);
>  int                     virDomainResumeFlags    (virDomainPtr domain,

The new flag also needs docs in the header of virDomainResumeFlags where
you should mention that it also needs the guest agent in various cases.

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 903ca5d..38926c4 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c

> @@ -1974,6 +1974,11 @@ qemuDomainResumeFlags(virDomainPtr dom,
>          goto endjob;
>      if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
>          goto endjob;
> +
> +    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)

>      ret = 0;
>  
>   endjob:
> 

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141106/2d16f378/attachment-0001.sig>