[libvirt] [PATCHv2 01/14] Check for domain liveness in qemuDomainObjExitMonitor

[John Ferlan]

On 01/07/2015 10:42 AM, Ján Tomko wrote:
> The domain might disappear during the time in monitor when
> the virDomainObjPtr is unlocked, so the caller needs to check
> if it's still alive.
> 
> Since most of the callers are going to need it, put the
> check inside qemuDomainObjExitMonitor and return -1 if
> the domain died in the meantime.
> ---
>  src/qemu/THREADS.txt   |  5 +++++
>  src/qemu/qemu_domain.c | 16 ++++++++++++++--
>  src/qemu/qemu_domain.h |  4 ++--
>  3 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/THREADS.txt b/src/qemu/THREADS.txt
> index add2a35..dfa372b 100644
> --- a/src/qemu/THREADS.txt
> +++ b/src/qemu/THREADS.txt
> @@ -160,6 +160,11 @@ To acquire the QEMU monitor lock
>      - Acquires the virDomainObjPtr lock
>  
>    These functions must not be used by an asynchronous job.
> +  Note that the virDomainObj is unlocked during the time in
> +  monitor and it can be changed, e.g. if QEMU dies, qemuProcessStop
> +  may free the live domain definition and put the persistent
> +  definition back in vm->def. The callers should check the return
> +  value of ExitMonitor to see if the domain is still alive.
>  
>  
>  To acquire the QEMU monitor lock as part of an asynchronous job
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 3d4023c..c942b02 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1586,11 +1586,23 @@ void qemuDomainObjEnterMonitor(virQEMUDriverPtr driver,
>  /* obj must NOT be locked before calling
>   *
>   * Should be paired with an earlier qemuDomainObjEnterMonitor() call
> + *
> + * Returns -1 if the domain is no longer alive after exiting the monitor.
> + * In that case, the caller should be careful when using obj's data,
> + * e.g. the live definition in vm->def has been freed by qemuProcessStop
> + * and replaced by the persistent definition, so pointers stolen
> + * from the live definition could no longer be valid.
>   */
> -void qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
> -                              virDomainObjPtr obj)
> +int qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
> +                             virDomainObjPtr obj)
>  {
>      qemuDomainObjExitMonitorInternal(driver, obj);
> +    if (!virDomainObjIsActive(obj)) {
> +        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> +                       _("domain is no longer running"));
> +        return -1;
> +    }
> +    return 0;

Do we have to worry about caller paths perhaps using virReportError and
this overwriting the other message for the last message? Does it really
matter?

ACK

John
>  }
>  
>  /*
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 6b52f03..fd91d83 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -245,8 +245,8 @@ void qemuDomainObjReleaseAsyncJob(virDomainObjPtr obj);
>  void qemuDomainObjEnterMonitor(virQEMUDriverPtr driver,
>                                 virDomainObjPtr obj)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> -void qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
> -                              virDomainObjPtr obj)
> +int qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
> +                             virDomainObjPtr obj)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>  int qemuDomainObjEnterMonitorAsync(virQEMUDriverPtr driver,
>                                     virDomainObjPtr obj,
>