On Tue, Oct 24, 2017 at 01:52:28PM +0800, xinhua.Cao wrote:
base on commit 2033e8cc119454bc4273e8a41e66c899c60ba58b and fe8f1c8b8650c8ab5e7d27338f4538582651bd14, we solve libvirt coredump problem, but it introduce a memory leak sense.
The first one is just a syntax sugar, it introduces no functional change.
the sense follow 1. one client register a domain event such as reboot event 2. and client was terminated unexpectly, then this client will not free at libvirtd service program. remoteDispatchConnectDomainEventCallbackRegisterAny reference the client, but when client was terminated before it call deRegisterAny, the reference of client will not reduced to zero. so the memory leak take place. this patch will deRegister all event when client was close.
Can you elaborate more on how does the client get terminated? Maybe the problem is that there is a way to terminate the client and not call the FreeFunc on it and the fact that it doesn't go through the right cleanup procedure should be what we should focus on? Also please wrap the commit message as any other commit. See `git log` for reference.
---
daemon/remote.c | 47 +++++++++++++++++++++++++++++------------------
1 file changed, 29 insertions(+), 18 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c
index 3f7d2d3..2b5a18b 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -1686,25 +1686,16 @@ void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int r
VIR_WARN("unexpected %s event deregister failure", name); \
} \
VIR_FREE(eventCallbacks); \
+ neventCallbacks = 0; \
This is OK, ACK to this hunk. But I think this should be in a separate patch, probably.
} while (0);
-/*
- * You must hold lock for at least the client
- * We don't free stuff here, merely disconnect the client's
- * network socket & resources.
- * We keep the libvirt connection open until any async
- * jobs have finished, then clean it up elsewhere
- */
-void remoteClientFreeFunc(void *data)
+static void
+remoteFreePrivCallbacks(void *data)
Why is it called Callbacks when it is not passed as a callback anywhere? Why does it take void *? Why does it not have a 'Client' in the name when it clearly works with a daemonClientPrivate data?
{
struct daemonClientPrivate *priv = data;
/* Deregister event delivery callback */
- if (priv->conn) {
- virIdentityPtr sysident = virIdentityGetSystem();
-
- virIdentitySetCurrent(sysident);
-
+ if (priv && priv->conn) {
DEREG_CB(priv->conn, priv->domainEventCallbacks,
priv->ndomainEventCallbacks,
virConnectDomainEventDeregisterAny, "domain");
@@ -1723,6 +1714,26 @@ void remoteClientFreeFunc(void *data)
DEREG_CB(priv->conn, priv->qemuEventCallbacks,
priv->nqemuEventCallbacks,
virConnectDomainQemuMonitorEventDeregister, "qemu monitor");
+ }
+}
+#undef DEREG_CB
+
+/*
+ * You must hold lock for at least the client
+ * We don't free stuff here, merely disconnect the client's
+ * network socket & resources.
+ * We keep the libvirt connection open until any async
+ * jobs have finished, then clean it up elsewhere
+ */
+void remoteClientFreeFunc(void *data)
+{
+ struct daemonClientPrivate *priv = data;
+
+ if (priv) {
+ virIdentityPtr sysident = virIdentityGetSystem();
+
+ virIdentitySetCurrent(sysident);
+ remoteFreePrivCallbacks(priv);
if (priv->closeRegistered) {
if (virConnectUnregisterCloseCallback(priv->conn,
Why don't you also remove this callback in the new function? Does the close event not get propagated when you move it there?
@@ -1734,18 +1745,18 @@ void remoteClientFreeFunc(void *data)
virIdentitySetCurrent(NULL);
virObjectUnref(sysident);
+ VIR_FREE(priv);
}
-
- VIR_FREE(priv);
}
-#undef DEREG_CB
-
static void remoteClientCloseFunc(virNetServerClientPtr client)
{
struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
- daemonRemoveAllClientStreams(priv->streams);
+ if (priv) {
Can it happen that priv is NULL? It should only be freed when the client is freed in which case this function should not be called at all. This is a warning light for me, if you encountered priv == NULL in this function, then it signals that there is probably a problem somewhere else as well.
+ daemonRemoveAllClientStreams(priv->streams); + remoteFreePrivCallbacks(priv); + } }
Generally, I'm OK with splitting the Free function to two of them, one doing the closing part and one freeing the data (similarly to what I suggested in another thread for virNetDaemonDispose() just now), but this patch does it in a very weird way.
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
