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

Re: [libvirt] [PATCH] Release conn lock before reporting errors



On Wed, Jul 22, 2009 at 11:13:45AM -0400, Laine Stump wrote:
> This is a follow-on to 528d37. It's fixing several other cases in
> datatypes.c where we try to report an error while holding the conn's
> lock, which can't work because the error reporting also tries to lock
> the conn.
> 
> ---
>  src/datatypes.c |  133 ++++++++++++++++++++++++++++++++++---------------------
>  1 files changed, 82 insertions(+), 51 deletions(-)

ACK, same pattern as previous patch.

Daniel

> 
> diff --git a/src/datatypes.c b/src/datatypes.c
> index ba5401d..c2030dd 100644
> --- a/src/datatypes.c
> +++ b/src/datatypes.c
> @@ -270,11 +270,13 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) {
>      /* TODO check the UUID */
>      if (ret == NULL) {
>          if (VIR_ALLOC(ret) < 0) {
> +            virMutexUnlock(&conn->lock);
>              virReportOOMError(conn);
>              goto error;
>          }
>          ret->name = strdup(name);
>          if (ret->name == NULL) {
> +            virMutexUnlock(&conn->lock);
>              virReportOOMError(conn);
>              goto error;
>          }
> @@ -285,6 +287,7 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) {
>              memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN);
>  
>          if (virHashAddEntry(conn->domains, name, ret) < 0) {
> +            virMutexUnlock(&conn->lock);
>              virLibConnError(conn, VIR_ERR_INTERNAL_ERROR,
>                              _("failed to add domain to connection hash table"));
>              goto error;
> @@ -299,7 +302,6 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) {
>      return(ret);
>  
>   error:
> -    virMutexUnlock(&conn->lock);
>      if (ret != NULL) {
>          VIR_FREE(ret->name);
>          VIR_FREE(ret);
> @@ -325,24 +327,28 @@ virReleaseDomain(virDomainPtr domain) {
>      DEBUG("release domain %p %s", domain, domain->name);
>  
>      /* TODO search by UUID first as they are better differenciators */
> -    if (virHashRemoveEntry(conn->domains, domain->name, NULL) < 0)
> +    if (virHashRemoveEntry(conn->domains, domain->name, NULL) < 0) {
> +        virMutexUnlock(&conn->lock);
>          virLibConnError(conn, VIR_ERR_INTERNAL_ERROR,
>                          _("domain missing from connection hash table"));
> +        conn = NULL;
> +    }
>  
>      domain->magic = -1;
>      domain->id = -1;
>      VIR_FREE(domain->name);
>      VIR_FREE(domain);
>  
> -    DEBUG("unref connection %p %d", conn, conn->refs);
> -    conn->refs--;
> -    if (conn->refs == 0) {
> -        virReleaseConnect(conn);
> -        /* Already unlocked mutex */
> -        return;
> +    if (conn) {
> +        DEBUG("unref connection %p %d", conn, conn->refs);
> +        conn->refs--;
> +        if (conn->refs == 0) {
> +            virReleaseConnect(conn);
> +            /* Already unlocked mutex */
> +            return;
> +        }
> +        virMutexUnlock(&conn->lock);
>      }
> -
> -    virMutexUnlock(&conn->lock);
>  }
>  
>  
> @@ -406,11 +412,13 @@ virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) {
>      /* TODO check the UUID */
>      if (ret == NULL) {
>          if (VIR_ALLOC(ret) < 0) {
> +            virMutexUnlock(&conn->lock);
>              virReportOOMError(conn);
>              goto error;
>          }
>          ret->name = strdup(name);
>          if (ret->name == NULL) {
> +            virMutexUnlock(&conn->lock);
>              virReportOOMError(conn);
>              goto error;
>          }
> @@ -420,6 +428,7 @@ virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) {
>              memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN);
>  
>          if (virHashAddEntry(conn->networks, name, ret) < 0) {
> +            virMutexUnlock(&conn->lock);
>              virLibConnError(conn, VIR_ERR_INTERNAL_ERROR,
>                              _("failed to add network to connection hash table"));
>              goto error;
> @@ -431,7 +440,6 @@ virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) {
>      return(ret);
>  
>   error:
> -    virMutexUnlock(&conn->lock);
>      if (ret != NULL) {
>          VIR_FREE(ret->name);
>          VIR_FREE(ret);
> @@ -457,23 +465,27 @@ virReleaseNetwork(virNetworkPtr network) {
>      DEBUG("release network %p %s", network, network->name);
>  
>      /* TODO search by UUID first as they are better differenciators */
> -    if (virHashRemoveEntry(conn->networks, network->name, NULL) < 0)
> +    if (virHashRemoveEntry(conn->networks, network->name, NULL) < 0) {
> +        virMutexUnlock(&conn->lock);
>          virLibConnError(conn, VIR_ERR_INTERNAL_ERROR,
>                          _("network missing from connection hash table"));
> +        conn = NULL;
> +    }
>  
>      network->magic = -1;
>      VIR_FREE(network->name);
>      VIR_FREE(network);
>  
> -    DEBUG("unref connection %p %d", conn, conn->refs);
> -    conn->refs--;
> -    if (conn->refs == 0) {
> -        virReleaseConnect(conn);
> -        /* Already unlocked mutex */
> -        return;
> +    if (conn) {
> +        DEBUG("unref connection %p %d", conn, conn->refs);
> +        conn->refs--;
> +        if (conn->refs == 0) {
> +            virReleaseConnect(conn);
> +            /* Already unlocked mutex */
> +            return;
> +        }
> +        virMutexUnlock(&conn->lock);
>      }
> -
> -    virMutexUnlock(&conn->lock);
>  }
>  
>  
> @@ -714,11 +726,13 @@ virGetStoragePool(virConnectPtr conn, const char *name, const unsigned char *uui
>      /* TODO check the UUID */
>      if (ret == NULL) {
>          if (VIR_ALLOC(ret) < 0) {
> +            virMutexUnlock(&conn->lock);
>              virReportOOMError(conn);
>              goto error;
>          }
>          ret->name = strdup(name);
>          if (ret->name == NULL) {
> +            virMutexUnlock(&conn->lock);
>              virReportOOMError(conn);
>              goto error;
>          }
> @@ -728,6 +742,7 @@ virGetStoragePool(virConnectPtr conn, const char *name, const unsigned char *uui
>              memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN);
>  
>          if (virHashAddEntry(conn->storagePools, name, ret) < 0) {
> +            virMutexUnlock(&conn->lock);
>              virLibConnError(conn, VIR_ERR_INTERNAL_ERROR,
>                              _("failed to add storage pool to connection hash table"));
>              goto error;
> @@ -739,7 +754,6 @@ virGetStoragePool(virConnectPtr conn, const char *name, const unsigned char *uui
>      return(ret);
>  
>  error:
> -    virMutexUnlock(&conn->lock);
>      if (ret != NULL) {
>          VIR_FREE(ret->name);
>          VIR_FREE(ret);
> @@ -766,23 +780,27 @@ virReleaseStoragePool(virStoragePoolPtr pool) {
>      DEBUG("release pool %p %s", pool, pool->name);
>  
>      /* TODO search by UUID first as they are better differenciators */
> -    if (virHashRemoveEntry(conn->storagePools, pool->name, NULL) < 0)
> +    if (virHashRemoveEntry(conn->storagePools, pool->name, NULL) < 0) {
> +        virMutexUnlock(&conn->lock);
>          virLibConnError(conn, VIR_ERR_INTERNAL_ERROR,
>                          _("pool missing from connection hash table"));
> +        conn = NULL;
> +    }
>  
>      pool->magic = -1;
>      VIR_FREE(pool->name);
>      VIR_FREE(pool);
>  
> -    DEBUG("unref connection %p %d", conn, conn->refs);
> -    conn->refs--;
> -    if (conn->refs == 0) {
> -        virReleaseConnect(conn);
> -        /* Already unlocked mutex */
> -        return;
> +    if (conn) {
> +        DEBUG("unref connection %p %d", conn, conn->refs);
> +        conn->refs--;
> +        if (conn->refs == 0) {
> +            virReleaseConnect(conn);
> +            /* Already unlocked mutex */
> +            return;
> +        }
> +        virMutexUnlock(&conn->lock);
>      }
> -
> -    virMutexUnlock(&conn->lock);
>  }
>  
>  
> @@ -845,16 +863,19 @@ virGetStorageVol(virConnectPtr conn, const char *pool, const char *name, const c
>      ret = (virStorageVolPtr) virHashLookup(conn->storageVols, key);
>      if (ret == NULL) {
>          if (VIR_ALLOC(ret) < 0) {
> +            virMutexUnlock(&conn->lock);
>              virReportOOMError(conn);
>              goto error;
>          }
>          ret->pool = strdup(pool);
>          if (ret->pool == NULL) {
> +            virMutexUnlock(&conn->lock);
>              virReportOOMError(conn);
>              goto error;
>          }
>          ret->name = strdup(name);
>          if (ret->name == NULL) {
> +            virMutexUnlock(&conn->lock);
>              virReportOOMError(conn);
>              goto error;
>          }
> @@ -864,6 +885,7 @@ virGetStorageVol(virConnectPtr conn, const char *pool, const char *name, const c
>          ret->conn = conn;
>  
>          if (virHashAddEntry(conn->storageVols, key, ret) < 0) {
> +            virMutexUnlock(&conn->lock);
>              virLibConnError(conn, VIR_ERR_INTERNAL_ERROR,
>                              _("failed to add storage vol to connection hash table"));
>              goto error;
> @@ -875,7 +897,6 @@ virGetStorageVol(virConnectPtr conn, const char *pool, const char *name, const c
>      return(ret);
>  
>  error:
> -    virMutexUnlock(&conn->lock);
>      if (ret != NULL) {
>          VIR_FREE(ret->name);
>          VIR_FREE(ret->pool);
> @@ -903,24 +924,28 @@ virReleaseStorageVol(virStorageVolPtr vol) {
>      DEBUG("release vol %p %s", vol, vol->name);
>  
>      /* TODO search by UUID first as they are better differenciators */
> -    if (virHashRemoveEntry(conn->storageVols, vol->key, NULL) < 0)
> +    if (virHashRemoveEntry(conn->storageVols, vol->key, NULL) < 0) {
> +        virMutexUnlock(&conn->lock);
>          virLibConnError(conn, VIR_ERR_INTERNAL_ERROR,
>                          _("vol missing from connection hash table"));
> +        conn = NULL;
> +    }
>  
>      vol->magic = -1;
>      VIR_FREE(vol->name);
>      VIR_FREE(vol->pool);
>      VIR_FREE(vol);
>  
> -    DEBUG("unref connection %p %d", conn, conn->refs);
> -    conn->refs--;
> -    if (conn->refs == 0) {
> -        virReleaseConnect(conn);
> -        /* Already unlocked mutex */
> -        return;
> +    if (conn) {
> +        DEBUG("unref connection %p %d", conn, conn->refs);
> +        conn->refs--;
> +        if (conn->refs == 0) {
> +            virReleaseConnect(conn);
> +            /* Already unlocked mutex */
> +            return;
> +        }
> +        virMutexUnlock(&conn->lock);
>      }
> -
> -    virMutexUnlock(&conn->lock);
>  }
>  
>  
> @@ -981,7 +1006,8 @@ virGetNodeDevice(virConnectPtr conn, const char *name)
>  
>      ret = (virNodeDevicePtr) virHashLookup(conn->nodeDevices, name);
>      if (ret == NULL) {
> -       if (VIR_ALLOC(ret) < 0) {
> +        if (VIR_ALLOC(ret) < 0) {
> +            virMutexUnlock(&conn->lock);
>              virReportOOMError(conn);
>              goto error;
>          }
> @@ -989,11 +1015,13 @@ virGetNodeDevice(virConnectPtr conn, const char *name)
>          ret->conn = conn;
>          ret->name = strdup(name);
>          if (ret->name == NULL) {
> +            virMutexUnlock(&conn->lock);
>              virReportOOMError(conn);
>              goto error;
>          }
>  
>          if (virHashAddEntry(conn->nodeDevices, name, ret) < 0) {
> +            virMutexUnlock(&conn->lock);
>              virLibConnError(conn, VIR_ERR_INTERNAL_ERROR,
>                              _("failed to add node dev to conn hash table"));
>              goto error;
> @@ -1005,7 +1033,6 @@ virGetNodeDevice(virConnectPtr conn, const char *name)
>      return(ret);
>  
>  error:
> -    virMutexUnlock(&conn->lock);
>      if (ret != NULL) {
>          VIR_FREE(ret->name);
>          VIR_FREE(ret);
> @@ -1031,24 +1058,28 @@ virReleaseNodeDevice(virNodeDevicePtr dev) {
>      virConnectPtr conn = dev->conn;
>      DEBUG("release dev %p %s", dev, dev->name);
>  
> -    if (virHashRemoveEntry(conn->nodeDevices, dev->name, NULL) < 0)
> +    if (virHashRemoveEntry(conn->nodeDevices, dev->name, NULL) < 0) {
> +        virMutexUnlock(&conn->lock);
>          virLibConnError(conn, VIR_ERR_INTERNAL_ERROR,
>                          _("dev missing from connection hash table"));
> +        conn = NULL;
> +    }
>  
>      dev->magic = -1;
>      VIR_FREE(dev->name);
>      VIR_FREE(dev->parent);
>      VIR_FREE(dev);
>  
> -    DEBUG("unref connection %p %d", conn, conn->refs);
> -    conn->refs--;
> -    if (conn->refs == 0) {
> -        virReleaseConnect(conn);
> -        /* Already unlocked mutex */
> -        return;
> +    if (conn) {
> +        DEBUG("unref connection %p %d", conn, conn->refs);
> +        conn->refs--;
> +        if (conn->refs == 0) {
> +            virReleaseConnect(conn);
> +            /* Already unlocked mutex */
> +            return;
> +        }
> +        virMutexUnlock(&conn->lock);
>      }
> -
> -    virMutexUnlock(&conn->lock);
>  }
>  
>  
> -- 
> 1.6.0.6
> 
> --
> Libvir-list mailing list
> Libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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