Re: [libvirt] [PATCH] Fix up connection reference counting.

[Chris Lalancette <clalance@xxxxxxxxxx>]

Daniel P. Berrange wrote:
> 
> ACK
> 

During testing of this patch, I actually found a small bug, so the final version
that I committed is attached.

-- 
Chris Lalancette

commit f5cc4a5a696680577eb6b2daaf7521bc5fcfbc5e
Author: Chris Lalancette <clalance@xxxxxxxxxx>
Date:   Tue Aug 11 15:48:59 2009 +0200

    Fix up connection reference counting.
    
    Currently the reference counting for connections is busted.  I
    first noticed it while trying to use virConnectRef; it would
    eventually cause a crash in the remote_internal driver, although
    that was really just a victim.  Really, we should only call the
    close callbacks on the methods when the references drop to 0.  To
    accomplish this, move all of the close callbacks into
    virUnrefConnect (since there are lots of internal users of that
    function), and arrange for virConnectClose to call that.
    
    V2: Make sure to drop the connection lock before we call the close
        callbacks, otherwise we could deadlock the daemon
    V3: Fix up a crash when we got an error from one of the drivers
    
    Signed-off-by: Chris Lalancette <clalance@xxxxxxxxxx>

diff --git a/src/datatypes.c b/src/datatypes.c
index 9d556c8..821c795 100644
--- a/src/datatypes.c
+++ b/src/datatypes.c
@@ -237,6 +237,26 @@ 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->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);
diff --git a/src/libvirt.c b/src/libvirt.c
index 5aa7f83..a764e63 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -1109,8 +1109,6 @@ do_open (const char *name,
     return ret;
 
 failed:
-    if (ret->driver) ret->driver->close (ret);
-
     /* Ensure a global error is set in case driver forgot */
     virSetGlobalError();
 
@@ -1231,19 +1229,7 @@ virConnectClose(virConnectPtr conn)
         return (-1);
     }
 
-    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);
-    conn->driver->close (conn);
-
-    if (virUnrefConnect(conn) < 0)
-        return (-1);
-    return (0);
+    return virUnrefConnect(conn);
 }
 
 /**

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