<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p><br>
</p>
<br>
<div class="moz-cite-prefix">在 2017/11/6 16:02, Martin Kletzander
写道:<br>
</div>
<blockquote type="cite" cite="mid:20171106080257.GB12337@wheatley">On
Mon, Nov 06, 2017 at 09:23:06AM +0800, xinhua.Cao wrote:
<br>
<blockquote type="cite">
<br>
<br>
在 2017/11/3 1:29, Martin Kletzander 写道:
<br>
<blockquote type="cite">On Tue, Oct 24, 2017 at 01:52:28PM
+0800, xinhua.Cao wrote:
<br>
<blockquote type="cite">base on commit
2033e8cc119454bc4273e8a41e66c899c60ba58b and
<br>
fe8f1c8b8650c8ab5e7d27338f4538582651bd14, we solve libvirt
coredump
<br>
problem, but it introduce a memory leak sense.
<br>
</blockquote>
<br>
The first one is just a syntax sugar, it introduces no
functional change.
<br>
<br>
</blockquote>
yes, this patch is OK. because first patch and second patch have
same
<br>
relationship, so I point those two patch.
<br>
<blockquote type="cite">
<blockquote type="cite">the sense follow
<br>
1. one client register a domain event such as reboot event
<br>
2. and client was terminated unexpectly, then this client
will not
<br>
free at libvirtd service program.
<br>
<br>
remoteDispatchConnectDomainEventCallbackRegisterAny
reference the
<br>
client, but when client was terminated before it call
deRegisterAny,
<br>
the reference of client will not reduced to zero. so the
memory leak
<br>
take place. this patch will deRegister all event when client
was close.
<br>
</blockquote>
<br>
Can you elaborate more on how does the client get terminated?
Maybe
<br>
the problem
<br>
is that there is a way to terminate the client and not call
the
<br>
FreeFunc on it
<br>
and the fact that it doesn't go through the right cleanup
procedure
<br>
should be
<br>
what we should focus on?
<br>
<br>
</blockquote>
such as kill -9 or client crash.
<br>
</blockquote>
<br>
Ok, then we should look at why is the current function not getting
<br>
called instead of calling part of it twice.
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">Also please wrap the commit message as
any other commit. See `git
<br>
log` for
<br>
reference.
<br>
</blockquote>
OK, it will be correct at v2 patch
<br>
<blockquote type="cite">
<br>
<blockquote type="cite">---
<br>
daemon/remote.c | 47
+++++++++++++++++++++++++++++------------------
<br>
1 file changed, 29 insertions(+), 18 deletions(-)
<br>
<br>
diff --git a/daemon/remote.c b/daemon/remote.c
<br>
index 3f7d2d3..2b5a18b 100644
<br>
--- a/daemon/remote.c
<br>
+++ b/daemon/remote.c
<br>
@@ -1686,25 +1686,16 @@ void
<br>
remoteRelayConnectionClosedEvent(virConnectPtr conn
ATTRIBUTE_UNUSED,
<br>
int r
<br>
VIR_WARN("unexpected %s event deregister
failure",
<br>
name); \
<br>
} \
<br>
VIR_FREE(eventCallbacks); \
<br>
+ neventCallbacks =
<br>
0; \
<br>
</blockquote>
<br>
This is OK, ACK to this hunk. But I think this should be in a
<br>
separate patch,
<br>
probably.
<br>
<br>
</blockquote>
OK, it will be at v2 patch
<br>
<blockquote type="cite">
<blockquote type="cite"> } while (0);
<br>
<br>
-/*
<br>
- * You must hold lock for at least the client
<br>
- * We don't free stuff here, merely disconnect the client's
<br>
- * network socket & resources.
<br>
- * We keep the libvirt connection open until any async
<br>
- * jobs have finished, then clean it up elsewhere
<br>
- */
<br>
-void remoteClientFreeFunc(void *data)
<br>
+static void
<br>
+remoteFreePrivCallbacks(void *data)
<br>
</blockquote>
<br>
Why is it called Callbacks when it is not passed as a callback
<br>
anywhere? Why
<br>
does it take void *? Why does it not have a 'Client' in the
name when it
<br>
clearly works with a daemonClientPrivate data?
<br>
<br>
</blockquote>
so can we use remoteClientFreePrivateCallbacks?
<br>
</blockquote>
<br>
As the function name? Is it a callback? From where?
<br>
</blockquote>
No,It is not a callback, but it clean all event callbacks of one
closed client.<br>
<blockquote type="cite" cite="mid:20171106080257.GB12337@wheatley">
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">{
<br>
struct daemonClientPrivate *priv = data;
<br>
<br>
/* Deregister event delivery callback */
<br>
- if (priv->conn) {
<br>
- virIdentityPtr sysident = virIdentityGetSystem();
<br>
-
<br>
- virIdentitySetCurrent(sysident);
<br>
-
<br>
+ if (priv && priv->conn) {
<br>
DEREG_CB(priv->conn,
priv->domainEventCallbacks,
<br>
priv->ndomainEventCallbacks,
<br>
virConnectDomainEventDeregisterAny,
"domain");
<br>
@@ -1723,6 +1714,26 @@ void remoteClientFreeFunc(void *data)
<br>
DEREG_CB(priv->conn, priv->qemuEventCallbacks,
<br>
priv->nqemuEventCallbacks,
<br>
virConnectDomainQemuMonitorEventDeregister,
"qemu
<br>
monitor");
<br>
+ }
<br>
+}
<br>
+#undef DEREG_CB
<br>
+
<br>
+/*
<br>
+ * You must hold lock for at least the client
<br>
+ * We don't free stuff here, merely disconnect the client's
<br>
+ * network socket & resources.
<br>
+ * We keep the libvirt connection open until any async
<br>
+ * jobs have finished, then clean it up elsewhere
<br>
+ */
<br>
+void remoteClientFreeFunc(void *data)
<br>
+{
<br>
+ struct daemonClientPrivate *priv = data;
<br>
+
<br>
+ if (priv) {
<br>
+ virIdentityPtr sysident = virIdentityGetSystem();
<br>
+
<br>
+ virIdentitySetCurrent(sysident);
<br>
+ remoteFreePrivCallbacks(priv);
<br>
<br>
if (priv->closeRegistered) {
<br>
if
(virConnectUnregisterCloseCallback(priv->conn,
<br>
</blockquote>
<br>
Why don't you also remove this callback in the new function?
Does the
<br>
close
<br>
event not get propagated when you move it there?
<br>
<br>
</blockquote>
OK, it will be at v2 patch
<br>
</blockquote>
<br>
Don't jump directly to sending another patch that might be wrong
again.
<br>
Sending 10 versions without a discussion wastes review bandwidth.
I was
<br>
not telling you what to do. I was just asking a question, maybe
I'm
<br>
wrong, maybe I misunderstood. Just answer would be fine so that
we hve
<br>
a discussion. But i think this whole thing will not be needed, as
I
<br>
wrote above, the problem will probably be why is the current
function
<br>
not get called, probably it doesn't need splitting at all.
<br>
<br>
</blockquote>
connectRegisterCloseCallback only set at vzHypervisorDriver. but vz
is unfamiliar to me.<br>
at qemu driver, connectRegisterCloseCallback is NULL, so it have no
effect on qemu driver.<br>
In my opinion, It likes domain evnet callback, so it can be called
at remoteClientCloseFunc. <br>
<span style="color: rgb(51, 51, 51); font-family: Arial, Helvetica,
sans-serif; font-size: 14px; font-style: normal;
font-variant-ligatures: normal; font-variant-caps: normal;
font-weight: normal; letter-spacing: normal; orphans: 2;
text-align: left; text-indent: 0px; text-transform: none;
white-space: normal; widows: 2; word-spacing: 0px;
-webkit-text-stroke-width: 0px; background-color: rgb(255, 255,
255); display: inline !important; float: none;"><span
class="Apple-converted-space"></span></span>
<blockquote type="cite" cite="mid:20171106080257.GB12337@wheatley">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">@@ -1734,18 +1745,18 @@ void
remoteClientFreeFunc(void *data)
<br>
<br>
virIdentitySetCurrent(NULL);
<br>
virObjectUnref(sysident);
<br>
+ VIR_FREE(priv);
<br>
}
<br>
-
<br>
- VIR_FREE(priv);
<br>
}
<br>
-#undef DEREG_CB
<br>
-
<br>
<br>
static void remoteClientCloseFunc(virNetServerClientPtr
client)
<br>
{
<br>
struct daemonClientPrivate *priv =
<br>
virNetServerClientGetPrivateData(client);
<br>
<br>
- daemonRemoveAllClientStreams(priv->streams);
<br>
+ if (priv) {
<br>
</blockquote>
<br>
Can it happen that priv is NULL? It should only be freed when
the
<br>
client is
<br>
freed in which case this function should not be called at all.
This is a
<br>
warning light for me, if you encountered priv == NULL in this
<br>
function, then it
<br>
signals that there is probably a problem somewhere else as
well.
<br>
<br>
</blockquote>
there have no way to take place "priv is NULL", I check it only
because
<br>
my habit. I will delete it at v2 patch.
<br>
</blockquote>
<br>
OK, that's fine in most cases, it's just that here it looked like
the
<br>
private data of the client might be NULL which would mean lot of
stuff
<br>
needs to be redone.
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">+
daemonRemoveAllClientStreams(priv->streams);
<br>
+ remoteFreePrivCallbacks(priv);
<br>
+ }
<br>
}
<br>
<br>
</blockquote>
<br>
Generally, I'm OK with splitting the Free function to two of
them, one
<br>
doing the
<br>
closing part and one freeing the data (similarly to what I
suggested
<br>
in another
<br>
thread for virNetDaemonDispose() just now), but this patch
does it in
<br>
a very
<br>
weird way.
<br>
</blockquote>
<br>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>