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@xxxxxxxxxx
> 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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list