Re: RFC: PATCH 1/5: cleanup connection ref counting

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

 



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  -=| 

--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]