Always close drivers when a virConnectPtr is released

virConnectClose calls virUnrefConnect which in turn closes
all open drivers when the refcount of that connection dropped
to zero. This works fine when you free all other objects that
hold a ref to the connection before you close it, because in
this case virUnrefConnect is the one that removes the last
ref to the connection.

But it doesn't work when you close the connection first before
freeing the other objects. This is because the other virUnref*
functions call virReleaseConnect when they detect that the
connection's refcount dropped to zero. In this case another
virUnref* function (different from virUnrefConnect) removes the
last ref to the connection. This results in not closing the
open drivers and leaking things that should have been cleaned
up in the driver close functions.

To fix this move the driver close calls to virReleaseConnect.

diff --git a/src/datatypes.c b/src/datatypes.c
index 46009aeb72acfe2778523c28a44dfa752f9e5275..9817538a0d10e3257c838dcf2586c5e7105467d3 100644 (file)
--- a/src/datatypes.c
+++ b/src/datatypes.c
@@ -247,6 +247,29 @@ failed:
 static void
 virReleaseConnect(virConnectPtr conn) {
     DEBUG("release connection %p", conn);
+
+    /* make sure to release the connection lock before we call the
+     * close() callbacks, otherwise we will deadlock if an error
+     * is raised by any of the callbacks */
+    virMutexUnlock(&conn->lock);
+
+    if (conn->networkDriver)
+        conn->networkDriver->close (conn);
+    if (conn->interfaceDriver)
+        conn->interfaceDriver->close (conn);
+    if (conn->storageDriver)
+        conn->storageDriver->close (conn);
+    if (conn->deviceMonitor)
+        conn->deviceMonitor->close (conn);
+    if (conn->secretDriver)
+        conn->secretDriver->close (conn);
+    if (conn->nwfilterDriver)
+        conn->nwfilterDriver->close (conn);
+    if (conn->driver)
+        conn->driver->close (conn);
+
+    virMutexLock(&conn->lock);
+
     if (conn->domains != NULL)
         virHashFree(conn->domains, (virHashDeallocator) virDomainFreeName);
     if (conn->networks != NULL)
@@ -295,30 +318,6 @@ virUnrefConnect(virConnectPtr conn) {
     conn->refs--;
     refs = conn->refs;
     if (refs == 0) {
-        /* make sure to release the connection lock before we call the
-         * close() callbacks, otherwise we will deadlock if an error
-         * is raised by any of the callbacks
-         */
-        virMutexUnlock(&conn->lock);
-        if (conn->networkDriver)
-            conn->networkDriver->close (conn);
-        if (conn->interfaceDriver)
-            conn->interfaceDriver->close (conn);
-        if (conn->storageDriver)
-            conn->storageDriver->close (conn);
-        if (conn->deviceMonitor)
-            conn->deviceMonitor->close (conn);
-        if (conn->secretDriver)
-            conn->secretDriver->close (conn);
-        if (conn->nwfilterDriver)
-            conn->nwfilterDriver->close (conn);
-        if (conn->driver)
-            conn->driver->close (conn);
-
-        /* reacquire the connection lock since virReleaseConnect expects
-         * it to already be held
-         */
-        virMutexLock(&conn->lock);
         virReleaseConnect(conn);
         /* Already unlocked mutex */
         return (0);