[libvirt] [PATCH] remote: Fix libvirtd service memory leak when libvirt client was terminated unexpectly
12:52 a.m.
base on commit 2033e8cc119454bc4273e8a41e66c899c60ba58b and
fe8f1c8b8650c8ab5e7d27338f4538582651bd14, we solve libvirt coredump problem, but it
introduce a memory leak sense.
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.
---
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; \
} 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)
{
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,
@@ -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) {
+ daemonRemoveAllClientStreams(priv->streams);
+ remoteFreePrivCallbacks(priv);
+ }
}
--
2.8.3
8:33 p.m.
New subject: [libvirt] 答复: [PATCH] remote: Fix libvirtd service memory leak when libvirt client was terminated unexpectly
ping
-----邮件原件-----
发件人: Caoxinhua
发送时间: 2017年10月24日 13:52
收件人: libvir-list(a)redhat.com; jferlan(a)redhat.com; mkletzan(a)redhat.com; berrange(a)redhat.com
抄送: Huangweidong (C); Yanqiangjun; weifuqiang; Wangjing (King, Euler); Caoxinhua
主题: [PATCH] remote: Fix libvirtd service memory leak when libvirt client was terminated
unexpectly
base on commit 2033e8cc119454bc4273e8a41e66c899c60ba58b and
fe8f1c8b8650c8ab5e7d27338f4538582651bd14, we solve libvirt coredump problem, but it
introduce a memory leak sense.
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.
---
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; \
} 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)
{
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,
@@ -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) {
+ daemonRemoveAllClientStreams(priv->streams);
+ remoteFreePrivCallbacks(priv);
+ }
}
--
2.8.3
8:08 p.m.
New subject: [libvirt] 答复: [PATCH] remote: Fix libvirtd service memory leak when libvirt client was terminated unexpectly
ping
-----邮件原件-----
发件人: Caoxinhua
发送时间: 2017年10月28日 9:33
收件人: Caoxinhua; libvir-list(a)redhat.com; jferlan(a)redhat.com; mkletzan(a)redhat.com;
berrange(a)redhat.com
抄送: Huangweidong (C); Yanqiangjun; weifuqiang; Wangjing (King, Euler)
主题: 答复: [PATCH] remote: Fix libvirtd service memory leak when libvirt client was
terminated unexpectly
ping
-----邮件原件-----
发件人: Caoxinhua
发送时间: 2017年10月24日 13:52
收件人: libvir-list(a)redhat.com; jferlan(a)redhat.com; mkletzan(a)redhat.com; berrange(a)redhat.com
抄送: Huangweidong (C); Yanqiangjun; weifuqiang; Wangjing (King, Euler); Caoxinhua
主题: [PATCH] remote: Fix libvirtd service memory leak when libvirt client was terminated
unexpectly
base on commit 2033e8cc119454bc4273e8a41e66c899c60ba58b and
fe8f1c8b8650c8ab5e7d27338f4538582651bd14, we solve libvirt coredump problem, but it
introduce a memory leak sense.
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.
---
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; \
} 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)
{
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,
@@ -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) {
+ daemonRemoveAllClientStreams(priv->streams);
+ remoteFreePrivCallbacks(priv);
+ }
}
--
2.8.3
12:29 p.m.
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.
7:23 p.m.
在 2017/11/3 1:29, Martin Kletzander 写道:
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.
yes, this patch is OK. because first patch and second patch have same
relationship, so I point those two patch.
> 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?
such as kill -9 or client crash.
Also please wrap the commit message as any other commit. See `git
log` for
reference.
OK, it will be correct at v2 patch
> ---
> 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.
OK, it will be at v2 patch
> } 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?
so can we use remoteClientFreePrivateCallbacks?
> {
> 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?
OK, it will be at v2 patch
> @@ -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.
there have no way to take place "priv is NULL", I check it only because
my habit. I will delete it at v2 patch.
> + 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.
2:02 a.m.
On Mon, Nov 06, 2017 at 09:23:06AM +0800, xinhua.Cao wrote:
在 2017/11/3 1:29, Martin Kletzander 写道:
> 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.
>
yes, this patch is OK. because first patch and second patch have same
relationship, so I point those two patch.
>> 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?
>
such as kill -9 or client crash.
Ok, then we should look at why is the current function not getting
called instead of calling part of it twice.
> Also please wrap the commit message as any other commit. See
`git
> log` for
> reference.
OK, it will be correct at v2 patch
>
>> ---
>> 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.
>
OK, it will be at v2 patch
>> } 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?
>
so can we use remoteClientFreePrivateCallbacks?
As the function name? Is it a callback? From where?
>> {
>> 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?
>
OK, it will be at v2 patch
Don't jump directly to sending another patch that might be wrong again.
Sending 10 versions without a discussion wastes review bandwidth. I was
not telling you what to do. I was just asking a question, maybe I'm
wrong, maybe I misunderstood. Just answer would be fine so that we hve
a discussion. But i think this whole thing will not be needed, as I
wrote above, the problem will probably be why is the current function
not get called, probably it doesn't need splitting at all.
>> @@ -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.
>
there have no way to take place "priv is NULL", I check it only because
my habit. I will delete it at v2 patch.
OK, that's fine in most cases, it's just that here it looked like the
private data of the client might be NULL which would mean lot of stuff
needs to be redone.
>> + 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.
5:29 a.m.
在 2017/11/6 16:02, Martin Kletzander 写道:
On Mon, Nov 06, 2017 at 09:23:06AM +0800, xinhua.Cao wrote:
>
>
> 在 2017/11/3 1:29, Martin Kletzander 写道:
>> 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.
>>
> yes, this patch is OK. because first patch and second patch have same
> relationship, so I point those two patch.
>>> 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?
>>
> such as kill -9 or client crash.
Ok, then we should look at why is the current function not getting
called instead of calling part of it twice.
>> Also please wrap the commit message as any other commit. See `git
>> log` for
>> reference.
> OK, it will be correct at v2 patch
>>
>>> ---
>>> 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.
>>
> OK, it will be at v2 patch
>>> } 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?
>>
> so can we use remoteClientFreePrivateCallbacks?
As the function name? Is it a callback? From where?
No,It is not a callback, but
it clean all event callbacks of one closed
client.
>>> {
>>> 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?
>>
> OK, it will be at v2 patch
Don't jump directly to sending another patch that might be wrong again.
Sending 10 versions without a discussion wastes review bandwidth. I was
not telling you what to do. I was just asking a question, maybe I'm
wrong, maybe I misunderstood. Just answer would be fine so that we hve
a discussion. But i think this whole thing will not be needed, as I
wrote above, the problem will probably be why is the current function
not get called, probably it doesn't need splitting at all.
connectRegisterCloseCallback only set at vzHypervisorDriver. but vz is
unfamiliar to me.
at qemu driver, connectRegisterCloseCallback is NULL, so it have no
effect on qemu driver.
In my opinion, It likes domain evnet callback, so it can be called at
remoteClientCloseFunc.
>>> @@ -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.
>>
> there have no way to take place "priv is NULL", I check it only because
> my habit. I will delete it at v2 patch.
OK, that's fine in most cases, it's just that here it looked like the
private data of the client might be NULL which would mean lot of stuff
needs to be redone.
>>> + 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.
>
>
2607
days inactive
2620
days old
6 comments
3 participants
participants (3)
-
Caoxinhua -
Martin Kletzander -
xinhua.Cao