[libvirt] [PATCH] 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. --- src/datatypes.c | 47 +++++++++++++++++++++++------------------------ 1 files changed, 23 insertions(+), 24 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index 46009ae..9817538 100644 --- 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); -- 1.7.0.4
On 11/24/2010 10:30 AM, Matthias Bolte wrote:
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. --- src/datatypes.c | 47 +++++++++++++++++++++++------------------------ 1 files changed, 23 insertions(+), 24 deletions(-)
ACK - virUnrefConnect still cleans up, by virtue of calling virReleaseConnect, but now anything else that also releases the connection also gets to clean up. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/11/24 Eric Blake <eblake@redhat.com>:
On 11/24/2010 10:30 AM, Matthias Bolte wrote:
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. --- src/datatypes.c | 47 +++++++++++++++++++++++------------------------ 1 files changed, 23 insertions(+), 24 deletions(-)
ACK - virUnrefConnect still cleans up, by virtue of calling virReleaseConnect, but now anything else that also releases the connection also gets to clean up.
Thanks, pushed. Matthias
participants (2)
-
Eric Blake -
Matthias Bolte