[Libvir] RFC: PATCH 1/5: cleanup connection ref counting
Daniel P. Berrange
berrange at redhat.com
Sat Jan 5 01:08:02 UTC 2008
The referencing counting code for Connect/Domain/Network
objects has many repeated codepaths, not all of which are
correct. eg, the virFreeDomain method forgets to release
networks when garbage collecting a virConnectPtr, and the
virFreeNetwork method forgets to release domains.
So I've moved the code for garbage collecting a virConnectPtr
object into a new virUnrefConnect() method which can be called
from virFreeConnect, virFreeDomain and virFreeNetwork.
I have also switched from xmlMutex to the POSIX standard
pthread_mutex_t object. In theory the former may give more
portability, but the patches which follow are also going
to require actual pthread_t objects, for which libxml has
no wrappers. Thus it is pointless using the xmlMutex
abstraction. The added plus, is that pthread_mutex_t objects
do not require any memory allocation, merely initialization
of their pre-allocated contents so it simplifies a little
code.
Finally, the code in hash.c for dealing with ref counting
seriously abused the 'goto' statement with multiple jumps
overlapping each other in a single method. This made it
very hard to follow. So I removed the use of goto in most
places so its only used in error cleanup paths, and not
for regular flow control.
Oh, and in cleaning up internal.h I found an odd declaration
of some functions from xs_internal.h which could have been
made static. So I made them static.
hash.c | 325 ++++++++++++++++++++++++++--------------------------------
internal.h | 55 ++++-----
xs_internal.c | 28 ----
3 files changed, 180 insertions(+), 228 deletions(-)
Dan.
diff -r 3a46c145fac4 src/hash.c
--- a/src/hash.c Thu Jan 03 21:48:19 2008 -0500
+++ b/src/hash.c Fri Jan 04 17:29:33 2008 -0500
@@ -25,6 +25,7 @@
#include <libxml/threads.h>
#include "internal.h"
#include "hash.h"
+#include <pthread.h>
#define MAX_HASH_LEN 8
@@ -677,60 +678,70 @@ virGetConnect(void) {
ret->networks = virHashCreate(20);
if (ret->networks == NULL)
goto failed;
- ret->hashes_mux = xmlNewMutex();
- if (ret->hashes_mux == NULL)
- goto failed;
-
- ret->uses = 1;
+
+ pthread_mutex_init(&ret->lock, NULL);
+
+ ret->refs = 1;
return(ret);
failed:
if (ret != NULL) {
- if (ret->domains != NULL)
- virHashFree(ret->domains, (virHashDeallocator) virDomainFreeName);
- if (ret->networks != NULL)
- virHashFree(ret->networks, (virHashDeallocator) virNetworkFreeName);
- if (ret->hashes_mux != NULL)
- xmlFreeMutex(ret->hashes_mux);
+ if (ret->domains != NULL)
+ virHashFree(ret->domains, (virHashDeallocator) virDomainFreeName);
+ if (ret->networks != NULL)
+ virHashFree(ret->networks, (virHashDeallocator) virNetworkFreeName);
+
+ pthread_mutex_destroy(&ret->lock);
free(ret);
}
return(NULL);
}
/**
- * virFreeConnect:
- * @conn: the hypervisor connection
- *
- * Release the connection. if the use count drops to zero, the structure is
- * actually freed.
- *
- * Returns the reference count or -1 in case of failure.
- */
-int
-virFreeConnect(virConnectPtr conn) {
- int ret;
-
- if ((!VIR_IS_CONNECT(conn)) || (conn->hashes_mux == NULL)) {
- virHashError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
- return(-1);
- }
- xmlMutexLock(conn->hashes_mux);
- conn->uses--;
- ret = conn->uses;
- if (ret > 0) {
- xmlMutexUnlock(conn->hashes_mux);
- return(ret);
- }
+ * Release all memory associated with a connection.
+ * The conn.lock mutex must be held prior to calling this
+ */
+int
+virUnrefConnect(virConnectPtr conn) {
+ conn->refs--;
+ if (conn->refs > 0)
+ return conn->refs;
if (conn->domains != NULL)
virHashFree(conn->domains, (virHashDeallocator) virDomainFreeName);
if (conn->networks != NULL)
virHashFree(conn->networks, (virHashDeallocator) virNetworkFreeName);
- if (conn->hashes_mux != NULL)
- xmlFreeMutex(conn->hashes_mux);
virResetError(&conn->err);
+
+ pthread_mutex_unlock(&conn->lock);
+ pthread_mutex_destroy(&conn->lock);
free(conn);
- return(0);
+
+ return 0;
+}
+
+/**
+ * virFreeConnect:
+ * @conn: the hypervisor connection
+ *
+ * Release the connection. if the use count drops to zero, the structure is
+ * actually freed.
+ *
+ * Returns the reference count or -1 in case of failure.
+ */
+int
+virFreeConnect(virConnectPtr conn) {
+ int refs;
+
+ if ((!VIR_IS_CONNECT(conn))) {
+ virHashError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
+ return(-1);
+ }
+ pthread_mutex_lock(&conn->lock);
+ refs = virUnrefConnect(conn);
+ if (refs > 0)
+ pthread_mutex_unlock(&conn->lock);
+ return (refs);
}
/**
@@ -750,57 +761,50 @@ __virGetDomain(virConnectPtr conn, const
__virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) {
virDomainPtr ret = NULL;
- if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL) ||
- (conn->hashes_mux == NULL)) {
+ if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) {
virHashError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
return(NULL);
}
- xmlMutexLock(conn->hashes_mux);
+ pthread_mutex_lock(&conn->lock);
/* TODO search by UUID first as they are better differenciators */
ret = (virDomainPtr) virHashLookup(conn->domains, name);
+ /* TODO check the UUID */
+ if (ret == NULL) {
+ ret = (virDomainPtr) calloc(1, sizeof(*ret));
+ if (ret == NULL) {
+ virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating domain"));
+ goto error;
+ }
+ ret->name = strdup(name);
+ if (ret->name == NULL) {
+ virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating domain"));
+ goto error;
+ }
+ ret->magic = VIR_DOMAIN_MAGIC;
+ ret->conn = conn;
+ ret->id = -1;
+ if (uuid != NULL)
+ memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN);
+
+ if (virHashAddEntry(conn->domains, name, ret) < 0) {
+ virHashError(conn, VIR_ERR_INTERNAL_ERROR,
+ _("failed to add domain to connection hash table"));
+ goto error;
+ }
+ conn->refs++;
+ }
+ ret->refs++;
+ pthread_mutex_unlock(&conn->lock);
+ return(ret);
+
+error:
+ pthread_mutex_unlock(&conn->lock);
if (ret != NULL) {
- /* TODO check the UUID */
- goto done;
- }
-
- /*
- * not found, allocate a new one
- */
- ret = (virDomainPtr) calloc(1, sizeof(virDomain));
- if (ret == NULL) {
- virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating domain"));
- goto error;
- }
- ret->name = strdup(name);
- if (ret->name == NULL) {
- virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating domain"));
- goto error;
- }
- ret->magic = VIR_DOMAIN_MAGIC;
- ret->conn = conn;
- ret->id = -1;
- if (uuid != NULL)
- memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN);
-
- if (virHashAddEntry(conn->domains, name, ret) < 0) {
- virHashError(conn, VIR_ERR_INTERNAL_ERROR,
- _("failed to add domain to connection hash table"));
- goto error;
- }
- conn->uses++;
-done:
- ret->uses++;
- xmlMutexUnlock(conn->hashes_mux);
- return(ret);
-
-error:
- xmlMutexUnlock(conn->hashes_mux);
- if (ret != NULL) {
- if (ret->name != NULL)
- free(ret->name );
- free(ret);
+ if (ret->name != NULL)
+ free(ret->name );
+ free(ret);
}
return(NULL);
}
@@ -817,29 +821,31 @@ error:
*/
int
virFreeDomain(virConnectPtr conn, virDomainPtr domain) {
- int ret = 0;
+ int refs;
if ((!VIR_IS_CONNECT(conn)) || (!VIR_IS_CONNECTED_DOMAIN(domain)) ||
- (domain->conn != conn) || (conn->hashes_mux == NULL)) {
+ (domain->conn != conn)) {
virHashError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
return(-1);
}
- xmlMutexLock(conn->hashes_mux);
+ pthread_mutex_lock(&conn->lock);
/*
* decrement the count for the domain
*/
- domain->uses--;
- ret = domain->uses;
- if (ret > 0)
- goto done;
+ domain->refs--;
+ refs = domain->refs;
+ if (refs > 0) {
+ pthread_mutex_unlock(&conn->lock);
+ return (refs);
+ }
/* TODO search by UUID first as they are better differenciators */
-
if (virHashRemoveEntry(conn->domains, domain->name, NULL) < 0) {
virHashError(conn, VIR_ERR_INTERNAL_ERROR,
- _("domain missing from connection hash table"));
- goto done;
+ _("domain missing from connection hash table"));
+ pthread_mutex_unlock(&conn->lock);
+ return (refs);
}
domain->magic = -1;
domain->id = -1;
@@ -847,23 +853,11 @@ virFreeDomain(virConnectPtr conn, virDom
free(domain->name);
free(domain);
- /*
- * decrement the count for the connection
- */
- conn->uses--;
- if (conn->uses > 0)
- goto done;
-
- if (conn->domains != NULL)
- virHashFree(conn->domains, (virHashDeallocator) virDomainFreeName);
- if (conn->hashes_mux != NULL)
- xmlFreeMutex(conn->hashes_mux);
- free(conn);
+
+ refs = virUnrefConnect(conn);
+ if (refs > 0)
+ pthread_mutex_unlock(&conn->lock);
return(0);
-
-done:
- xmlMutexUnlock(conn->hashes_mux);
- return(ret);
}
/**
@@ -883,56 +877,48 @@ __virGetNetwork(virConnectPtr conn, cons
__virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) {
virNetworkPtr ret = NULL;
- if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL) ||
- (conn->hashes_mux == NULL)) {
+ if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) {
virHashError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
return(NULL);
}
- xmlMutexLock(conn->hashes_mux);
+ pthread_mutex_lock(&conn->lock);
/* TODO search by UUID first as they are better differenciators */
ret = (virNetworkPtr) virHashLookup(conn->networks, name);
+ if (ret == NULL) {
+ ret = (virNetworkPtr) calloc(1, sizeof(*ret));
+ if (ret == NULL) {
+ virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating network"));
+ goto error;
+ }
+ ret->name = strdup(name);
+ if (ret->name == NULL) {
+ virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating network"));
+ goto error;
+ }
+ ret->magic = VIR_NETWORK_MAGIC;
+ ret->conn = conn;
+ if (uuid != NULL)
+ memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN);
+
+ if (virHashAddEntry(conn->networks, name, ret) < 0) {
+ virHashError(conn, VIR_ERR_INTERNAL_ERROR,
+ _("failed to add network to connection hash table"));
+ goto error;
+ }
+ conn->refs++;
+ }
+ ret->refs++;
+ pthread_mutex_unlock(&conn->lock);
+ return(ret);
+
+error:
+ pthread_mutex_unlock(&conn->lock);
if (ret != NULL) {
- /* TODO check the UUID */
- goto done;
- }
-
- /*
- * not found, allocate a new one
- */
- ret = (virNetworkPtr) calloc(1, sizeof(virNetwork));
- if (ret == NULL) {
- virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating network"));
- goto error;
- }
- ret->name = strdup(name);
- if (ret->name == NULL) {
- virHashError(conn, VIR_ERR_NO_MEMORY, _("allocating network"));
- goto error;
- }
- ret->magic = VIR_NETWORK_MAGIC;
- ret->conn = conn;
- if (uuid != NULL)
- memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN);
-
- if (virHashAddEntry(conn->networks, name, ret) < 0) {
- virHashError(conn, VIR_ERR_INTERNAL_ERROR,
- _("failed to add network to connection hash table"));
- goto error;
- }
- conn->uses++;
-done:
- ret->uses++;
- xmlMutexUnlock(conn->hashes_mux);
- return(ret);
-
-error:
- xmlMutexUnlock(conn->hashes_mux);
- if (ret != NULL) {
- if (ret->name != NULL)
- free(ret->name );
- free(ret);
+ if (ret->name != NULL)
+ free(ret->name );
+ free(ret);
}
return(NULL);
}
@@ -949,53 +935,44 @@ error:
*/
int
virFreeNetwork(virConnectPtr conn, virNetworkPtr network) {
- int ret = 0;
+ int refs;
if ((!VIR_IS_CONNECT(conn)) || (!VIR_IS_CONNECTED_NETWORK(network)) ||
- (network->conn != conn) || (conn->hashes_mux == NULL)) {
+ (network->conn != conn)) {
virHashError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
return(-1);
}
- xmlMutexLock(conn->hashes_mux);
+ pthread_mutex_lock(&conn->lock);
/*
* decrement the count for the network
*/
- network->uses--;
- ret = network->uses;
- if (ret > 0)
- goto done;
+ network->refs--;
+ refs = network->refs;
+ if (refs > 0) {
+ pthread_mutex_unlock(&conn->lock);
+ return (refs);
+ }
/* TODO search by UUID first as they are better differenciators */
-
if (virHashRemoveEntry(conn->networks, network->name, NULL) < 0) {
virHashError(conn, VIR_ERR_INTERNAL_ERROR,
_("network missing from connection hash table"));
- goto done;
+ pthread_mutex_unlock(&conn->lock);
+ return (refs);
}
network->magic = -1;
if (network->name)
free(network->name);
free(network);
- /*
- * decrement the count for the connection
- */
- conn->uses--;
- if (conn->uses > 0)
- goto done;
-
- if (conn->networks != NULL)
- virHashFree(conn->networks, (virHashDeallocator) virNetworkFreeName);
- if (conn->hashes_mux != NULL)
- xmlFreeMutex(conn->hashes_mux);
- free(conn);
+ refs = virUnrefConnect(conn);
+ if (refs > 0)
+ pthread_mutex_unlock(&conn->lock);
return(0);
-
-done:
- xmlMutexUnlock(conn->hashes_mux);
- return(ret);
-}
+}
+
+
/*
* Local variables:
diff -r 3a46c145fac4 src/internal.h
--- a/src/internal.h Thu Jan 03 21:48:19 2008 -0500
+++ b/src/internal.h Fri Jan 04 17:29:33 2008 -0500
@@ -132,8 +132,8 @@ extern "C" {
*/
struct _virConnect {
unsigned int magic; /* specific value to check */
-
- int uses; /* reference count */
+ int flags; /* a set of connection flags */
+ char *name; /* connection URI */
/* The underlying hypervisor driver and network driver. */
virDriverPtr driver;
@@ -151,12 +151,16 @@ struct _virConnect {
virErrorFunc handler; /* associated handlet */
void *userData; /* the user data */
- /* misc */
- xmlMutexPtr hashes_mux;/* a mutex to protect the domain and networks hash tables */
- virHashTablePtr domains;/* hash table for known domains */
- virHashTablePtr networks;/* hash table for known domains */
- int flags; /* a set of connection flags */
- char *name; /* connection URI */
+ /*
+ * The lock mutex must be acquired before accessing/changing
+ * any of members following this point, or changing the ref
+ * count of any virDomain/virNetwork object associated with
+ * this connection
+ */
+ pthread_mutex_t lock;
+ virHashTablePtr domains; /* hash table for known domains */
+ virHashTablePtr networks; /* hash table for known domains */
+ int refs; /* reference count */
};
/**
@@ -166,7 +170,7 @@ struct _virConnect {
*/
struct _virDomain {
unsigned int magic; /* specific value to check */
- int uses; /* reference count */
+ int refs; /* reference count */
virConnectPtr conn; /* pointer back to the connection */
char *name; /* the domain external name */
int id; /* the domain ID */
@@ -180,18 +184,12 @@ struct _virDomain {
*/
struct _virNetwork {
unsigned int magic; /* specific value to check */
- int uses; /* reference count */
+ int refs; /* reference count */
virConnectPtr conn; /* pointer back to the connection */
char *name; /* the network external name */
unsigned char uuid[VIR_UUID_BUFLEN]; /* the network unique identifier */
};
-/*
-* Internal routines
-*/
-char *virDomainGetVM(virDomainPtr domain);
-char *virDomainGetVMInfo(virDomainPtr domain,
- const char *vm, const char *name);
/************************************************************************
* *
@@ -217,18 +215,19 @@ const char *__virErrorMsg(virErrorNumber
* *
************************************************************************/
-virConnectPtr virGetConnect (void);
-int virFreeConnect (virConnectPtr conn);
-virDomainPtr __virGetDomain (virConnectPtr conn,
- const char *name,
- const unsigned char *uuid);
-int virFreeDomain (virConnectPtr conn,
- virDomainPtr domain);
-virNetworkPtr __virGetNetwork (virConnectPtr conn,
- const char *name,
- const unsigned char *uuid);
-int virFreeNetwork (virConnectPtr conn,
- virNetworkPtr domain);
+virConnectPtr virGetConnect(void);
+int virUnrefConnect(virConnectPtr conn);
+int virFreeConnect(virConnectPtr conn);
+virDomainPtr __virGetDomain(virConnectPtr conn,
+ const char *name,
+ const unsigned char *uuid);
+int virFreeDomain(virConnectPtr conn,
+ virDomainPtr domain);
+virNetworkPtr __virGetNetwork(virConnectPtr conn,
+ const char *name,
+ const unsigned char *uuid);
+int virFreeNetwork(virConnectPtr conn,
+ virNetworkPtr domain);
#define virGetDomain(c,n,u) __virGetDomain((c),(n),(u))
#define virGetNetwork(c,n,u) __virGetNetwork((c),(n),(u))
diff -r 3a46c145fac4 src/xs_internal.c
--- a/src/xs_internal.c Thu Jan 03 21:48:19 2008 -0500
+++ b/src/xs_internal.c Fri Jan 04 17:29:33 2008 -0500
@@ -227,7 +227,7 @@ virDomainDoStoreWrite(virDomainPtr domai
*
* Returns the new string or NULL in case of error
*/
-char *
+static char *
virDomainGetVM(virDomainPtr domain)
{
char *vm;
@@ -261,7 +261,7 @@ virDomainGetVM(virDomainPtr domain)
*
* Returns the new string or NULL in case of error
*/
-char *
+static char *
virDomainGetVMInfo(virDomainPtr domain, const char *vm, const char *name)
{
char s[256];
@@ -284,30 +284,6 @@ virDomainGetVMInfo(virDomainPtr domain,
return (ret);
}
-#if 0
-/**
- * virConnectCheckStoreID:
- * @conn: pointer to the hypervisor connection
- * @id: the id number as returned from Xenstore
- *
- * the xenstore sometimes list non-running domains, double check
- * from the hypervisor if we have direct access
- *
- * Returns -1 if the check failed, 0 if successful or not possible to check
- */
-static int
-virConnectCheckStoreID(virConnectPtr conn, int id)
-{
- if (conn->id >= 0) {
- int tmp;
-
- tmp = xenHypervisorCheckID(conn, id);
- if (tmp < 0)
- return (-1);
- }
- return (0);
-}
-#endif
#endif /* ! PROXY */
/************************************************************************
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
More information about the libvir-list
mailing list