[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/2] qemu: Avoid deadlock in autodestroy



On Mon, Feb 18, 2013 at 05:07:45PM +0100, Jiri Denemark wrote:
> Since closeCallbacks were turned into virObjectLockable, we can no
> longer call virQEMUCloseCallbacks APIs from within a registered close
> callback.
> ---
>  src/qemu/qemu_conf.c    | 19 ++++---------------
>  src/qemu/qemu_conf.h    |  4 ++++
>  src/qemu/qemu_domain.h  |  1 +
>  src/qemu/qemu_process.c | 10 +++++++++-
>  4 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index edadf36..fc8e152 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -825,8 +825,6 @@ virQEMUCloseCallbacksRunOne(void *payload,
>  {
>      struct virQEMUCloseCallbacksData *data = opaque;
>      qemuDriverCloseDefPtr closeDef = payload;
> -    unsigned char uuid[VIR_UUID_BUFLEN];
> -    char uuidstr[VIR_UUID_STRING_BUFLEN];
>      virDomainObjPtr dom;
>  
>      VIR_DEBUG("conn=%p, thisconn=%p, uuid=%s, cb=%p",
> @@ -835,18 +833,9 @@ virQEMUCloseCallbacksRunOne(void *payload,
>      if (data->conn != closeDef->conn || !closeDef->cb)
>          return;
>  
> -    if (virUUIDParse(name, uuid) < 0) {
> -        VIR_WARN("Failed to parse %s", (const char *)name);
> -        return;
> -    }
> -    /* We need to reformat uuidstr, because closeDef->cb
> -     * might cause the current hash entry to be removed,
> -     * which means 'name' will have been free()d
> -     */
> -    virUUIDFormat(uuid, uuidstr);
> -
> -    if (!(dom = virDomainObjListFindByUUID(data->driver->domains, uuid))) {
> -        VIR_DEBUG("No domain object with UUID %s", uuidstr);
> +    if (!(dom = virDomainObjListFindByUUID(data->driver->domains, name))) {
> +        VIR_DEBUG("No domain object with UUID %s",
> +                  (const char *) name);
>          return;
>      }
>  
> @@ -854,7 +843,7 @@ virQEMUCloseCallbacksRunOne(void *payload,
>      if (dom)
>          virObjectUnlock(dom);
>  
> -    virHashRemoveEntry(data->list, uuidstr);
> +    virHashRemoveEntry(data->list, name);
>  }
>  
>  void
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 72380dc..b5a3281 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -254,6 +254,10 @@ struct qemuDomainDiskInfo {
>      int io_status;
>  };
>  
> +/*
> + * To avoid a certain deadlock this callback must never call any
> + * virQEMUCloseCallbacks* API.
> + */
>  typedef virDomainObjPtr (*virQEMUCloseCallback)(virQEMUDriverPtr driver,
>                                                  virDomainObjPtr vm,
>                                                  virConnectPtr conn);
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 905b099..c9d5f8b 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -137,6 +137,7 @@ struct _qemuDomainObjPrivate {
>  
>      bool gotShutdown;
>      bool beingDestroyed;
> +    bool autoDestroyed;
>      char *pidfile;
>  
>      int nvcpupids;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index f987618..74406a6 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4263,7 +4263,8 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>      qemuDomainCleanupRun(driver, vm);
>  
>      /* Stop autodestroy in case guest is restarted */
> -    qemuProcessAutoDestroyRemove(driver, vm);
> +    if (!priv->autoDestroyed)
> +        qemuProcessAutoDestroyRemove(driver, vm);
>  
>      /* now that we know it's stopped call the hook if present */
>      if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) {
> @@ -4647,8 +4648,15 @@ qemuProcessAutoDestroy(virQEMUDriverPtr driver,
>          goto cleanup;
>  
>      VIR_DEBUG("Killing domain");
> +
> +    /* We need to prevent qemuProcessStop from removing this function from
> +     * closeCallbacks since that would cause a deadlock.
> +     */
> +    priv->autoDestroyed = true;
>      qemuProcessStop(driver, dom, VIR_DOMAIN_SHUTOFF_DESTROYED,
>                      VIR_QEMU_PROCESS_STOP_MIGRATED);
> +    priv->autoDestroyed = false;
> +
>      virDomainAuditStop(dom, "destroyed");
>      event = virDomainEventNewFromObj(dom,
>                                       VIR_DOMAIN_EVENT_STOPPED,

Slightly gross, but it'll do for now.

ACK

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]