[libvirt] [PATCH v3 1/2] remote: set neventCallbacks to zero at DEREG_CB
set neventCallbacks to zero after VIR_FREE(eventCallbacks) was called --- daemon/remote.c | 1 + 1 file changed, 1 insertion(+) diff --git a/daemon/remote.c b/daemon/remote.c index 07557e9..cbcb6e8 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1686,6 +1686,7 @@ void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int r VIR_WARN("unexpected %s event deregister failure", name); \ } \ VIR_FREE(eventCallbacks); \ + neventCallbacks = 0; \ } while (0); /* -- 2.8.3
base on commit fe8f1c8b8650c8ab5e7d27338f4538582651bd14, we solve libvirt coredump problem, but it introduce a memory leak sense: 1. one client register a domain event such as reboot event 2. and client was terminated unexpectly, such as kill -9, then this client object 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 callbacks when client was close. --- daemon/remote.c | 62 +++++++++++++++++++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index cbcb6e8..2073534 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1689,6 +1689,37 @@ void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int r neventCallbacks = 0; \ } while (0); +static void +remoteClientFreePrivateCallbacks(struct daemonClientPrivate *priv) +{ + DEREG_CB(priv->conn, priv->domainEventCallbacks, + priv->ndomainEventCallbacks, + virConnectDomainEventDeregisterAny, "domain"); + DEREG_CB(priv->conn, priv->networkEventCallbacks, + priv->nnetworkEventCallbacks, + virConnectNetworkEventDeregisterAny, "network"); + DEREG_CB(priv->conn, priv->storageEventCallbacks, + priv->nstorageEventCallbacks, + virConnectStoragePoolEventDeregisterAny, "storage"); + DEREG_CB(priv->conn, priv->nodeDeviceEventCallbacks, + priv->nnodeDeviceEventCallbacks, + virConnectNodeDeviceEventDeregisterAny, "node device"); + DEREG_CB(priv->conn, priv->secretEventCallbacks, + priv->nsecretEventCallbacks, + virConnectSecretEventDeregisterAny, "secret"); + DEREG_CB(priv->conn, priv->qemuEventCallbacks, + priv->nqemuEventCallbacks, + virConnectDomainQemuMonitorEventDeregister, "qemu monitor"); + + if (priv->closeRegistered) { + if (virConnectUnregisterCloseCallback(priv->conn, + remoteRelayConnectionClosedEvent) < 0) + VIR_WARN("unexpected close callback event deregister failure"); + } +} +#undef DEREG_CB + + /* * You must hold lock for at least the client * We don't free stuff here, merely disconnect the client's @@ -1706,40 +1737,17 @@ void remoteClientFreeFunc(void *data) virIdentitySetCurrent(sysident); - DEREG_CB(priv->conn, priv->domainEventCallbacks, - priv->ndomainEventCallbacks, - virConnectDomainEventDeregisterAny, "domain"); - DEREG_CB(priv->conn, priv->networkEventCallbacks, - priv->nnetworkEventCallbacks, - virConnectNetworkEventDeregisterAny, "network"); - DEREG_CB(priv->conn, priv->storageEventCallbacks, - priv->nstorageEventCallbacks, - virConnectStoragePoolEventDeregisterAny, "storage"); - DEREG_CB(priv->conn, priv->nodeDeviceEventCallbacks, - priv->nnodeDeviceEventCallbacks, - virConnectNodeDeviceEventDeregisterAny, "node device"); - DEREG_CB(priv->conn, priv->secretEventCallbacks, - priv->nsecretEventCallbacks, - virConnectSecretEventDeregisterAny, "secret"); - DEREG_CB(priv->conn, priv->qemuEventCallbacks, - priv->nqemuEventCallbacks, - virConnectDomainQemuMonitorEventDeregister, "qemu monitor"); - - if (priv->closeRegistered) { - if (virConnectUnregisterCloseCallback(priv->conn, - remoteRelayConnectionClosedEvent) < 0) - VIR_WARN("unexpected close callback event deregister failure"); - } + remoteClientFreePrivateCallbacks(priv); virConnectClose(priv->conn); virIdentitySetCurrent(NULL); virObjectUnref(sysident); - } + } VIR_FREE(priv); + } -#undef DEREG_CB static void remoteClientCloseFunc(virNetServerClientPtr client) @@ -1747,6 +1755,8 @@ static void remoteClientCloseFunc(virNetServerClientPtr client) struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); daemonRemoveAllClientStreams(priv->streams); + if (priv->conn) + remoteClientFreePrivateCallbacks(priv); } -- 2.8.3
On 11/11/2017 03:30 AM, xinhua.Cao wrote:
base on commit fe8f1c8b8650c8ab5e7d27338f4538582651bd14, we solve libvirt coredump problem, but it introduce a memory leak sense:
1. one client register a domain event such as reboot event 2. and client was terminated unexpectly, such as kill -9, then this client object 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 callbacks when client was close. --- daemon/remote.c | 62 +++++++++++++++++++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 26 deletions(-)
See my comments from the v2 series.... John
diff --git a/daemon/remote.c b/daemon/remote.c index cbcb6e8..2073534 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1689,6 +1689,37 @@ void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int r neventCallbacks = 0; \ } while (0);
+static void +remoteClientFreePrivateCallbacks(struct daemonClientPrivate *priv) +{ + DEREG_CB(priv->conn, priv->domainEventCallbacks, + priv->ndomainEventCallbacks, + virConnectDomainEventDeregisterAny, "domain"); + DEREG_CB(priv->conn, priv->networkEventCallbacks, + priv->nnetworkEventCallbacks, + virConnectNetworkEventDeregisterAny, "network"); + DEREG_CB(priv->conn, priv->storageEventCallbacks, + priv->nstorageEventCallbacks, + virConnectStoragePoolEventDeregisterAny, "storage"); + DEREG_CB(priv->conn, priv->nodeDeviceEventCallbacks, + priv->nnodeDeviceEventCallbacks, + virConnectNodeDeviceEventDeregisterAny, "node device"); + DEREG_CB(priv->conn, priv->secretEventCallbacks, + priv->nsecretEventCallbacks, + virConnectSecretEventDeregisterAny, "secret"); + DEREG_CB(priv->conn, priv->qemuEventCallbacks, + priv->nqemuEventCallbacks, + virConnectDomainQemuMonitorEventDeregister, "qemu monitor"); + + if (priv->closeRegistered) { + if (virConnectUnregisterCloseCallback(priv->conn, + remoteRelayConnectionClosedEvent) < 0) + VIR_WARN("unexpected close callback event deregister failure"); + } +} +#undef DEREG_CB + + /* * You must hold lock for at least the client * We don't free stuff here, merely disconnect the client's @@ -1706,40 +1737,17 @@ void remoteClientFreeFunc(void *data)
virIdentitySetCurrent(sysident);
- DEREG_CB(priv->conn, priv->domainEventCallbacks, - priv->ndomainEventCallbacks, - virConnectDomainEventDeregisterAny, "domain"); - DEREG_CB(priv->conn, priv->networkEventCallbacks, - priv->nnetworkEventCallbacks, - virConnectNetworkEventDeregisterAny, "network"); - DEREG_CB(priv->conn, priv->storageEventCallbacks, - priv->nstorageEventCallbacks, - virConnectStoragePoolEventDeregisterAny, "storage"); - DEREG_CB(priv->conn, priv->nodeDeviceEventCallbacks, - priv->nnodeDeviceEventCallbacks, - virConnectNodeDeviceEventDeregisterAny, "node device"); - DEREG_CB(priv->conn, priv->secretEventCallbacks, - priv->nsecretEventCallbacks, - virConnectSecretEventDeregisterAny, "secret"); - DEREG_CB(priv->conn, priv->qemuEventCallbacks, - priv->nqemuEventCallbacks, - virConnectDomainQemuMonitorEventDeregister, "qemu monitor"); - - if (priv->closeRegistered) { - if (virConnectUnregisterCloseCallback(priv->conn, - remoteRelayConnectionClosedEvent) < 0) - VIR_WARN("unexpected close callback event deregister failure"); - } + remoteClientFreePrivateCallbacks(priv);
virConnectClose(priv->conn);
virIdentitySetCurrent(NULL); virObjectUnref(sysident); - }
+ } VIR_FREE(priv); + } -#undef DEREG_CB
static void remoteClientCloseFunc(virNetServerClientPtr client) @@ -1747,6 +1755,8 @@ static void remoteClientCloseFunc(virNetServerClientPtr client) struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
daemonRemoveAllClientStreams(priv->streams); + if (priv->conn) + remoteClientFreePrivateCallbacks(priv); }
participants (2)
-
John Ferlan -
xinhua.Cao