2:20 a.m.
在 2017/11/11 5:43, John Ferlan 写道:
On 11/06/2017 12:47 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,
unexpectedly
> 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.
What's the difference between the path that would terminate normally and
the one that terminates abnormally. It seems to me there might be (an)
extra Ref(s) on the client which prevents virNetServerClientDispose from
calling it's @privateDataFreeFunc which would call remoteClientFreeFunc
since remoteClientInitHook worked properly.
That's what needs to be found.
If client terminate normally, client would call
virConnectDomainEventDeregisterAny to
clear eventCallbacks. Then libvirt daemon will call
remoteDispatchConnectDomainEventDeregisterAny
-> remoteEventCallbackFree -> virObjectUnref(callback->client) to free
eventCallbacks and unRef client.
just think if client don't call virConnectDomainEventDeregisterAny and
be killed. then there is no way to call call
remoteDispatchConnectDomainEventDeregisterAny -> remoteEventCallbackFree
-> virObjectUnref(callback->client)
so client's ref can't down to zero. so remoteClientFreeFunc can't be
called. then client object memory leak.
The difference is call remoteDispatchConnectDomainEventDeregisterAny or
don't call remoteDispatchConnectDomainEventDeregisterAny
to Unref(callback->client).
here is my test code
#include<stdio.h>
#include<pthread.h>
#include<libvirt/libvirt.h>
int _DomainStatusCallback(virConnectPtr conn, virDomainPtr dom, int event,
int detail, void *opaque);
void timeOutCallback(int timer, void *opaque);
int run = 1;
int _DomainStatusCallback(virConnectPtr conn, virDomainPtr dom, int event,
int detail, void *opaque)
{
char uuid[128] = { 0 };
(void) virDomainGetUUIDString(dom, uuid);
printf("domain %s come a event %d\n", uuid, event);
}
void _timeOutCallback(int timer, void *opaque)
{
printf("time out is comming\n");
}
void *event_thread(void *opaque)
{
int i =0;
if (virEventRegisterDefaultImpl() < 0)
{
return NULL;
}
virConnectPtr conn = virConnectOpen("qemu:///system");
if (conn == NULL)
{
return NULL;
}
int callback = virConnectDomainEventRegisterAny(conn, NULL,
VIR_DOMAIN_EVENT_ID_LIFECYCLE,
_DomainStatusCallback, NULL, NULL);
int timeout = virEventAddTimeout(1000,_timeOutCallback,NULL,NULL);
while (run && virConnectIsAlive(conn) == 1)
{
virEventRunDefaultImpl();
}
sleep(5);
virConnectDomainEventDeregisterAny(conn, callback);
virConnectClose(conn);
}
int main(int argc, char *argv[])
{
pthread_t pid;
int ret = 0;
ret = pthread_create(&pid, NULL, event_thread, NULL);
sleep(100);
run = 0;
sleep(10);
}
when I don't call remoteClientFreePrivateCallbacks at remoteClientCloseFunc,
here is client refs change when I kill the client.
(gdb) c
Continuing.
Hardware watchpoint 6: *(int *) 0x5637edc888f4
Old value = 4
New value = 5
virObjectRef (anyobj=anyobj@entry=0x5637edc888f0) at util/virobject.c:296
296 PROBE(OBJECT_REF, "obj=%p", obj);
(gdb) bt
#0 virObjectRef (anyobj=anyobj@entry=0x5637edc888f0) at
util/virobject.c:296
#1 0x00005637ec61d643 in
remoteDispatchConnectDomainEventCallbackRegisterAny (server=<optimized
out>, msg=<optimized out>,
ret=0x7f054c0008e0, args=0x7f054c0008c0, rerr=0x7f057ba6cc90,
client=0x5637edc888f0) at remote.c:4418
#2 remoteDispatchConnectDomainEventCallbackRegisterAnyHelper
(server=<optimized out>, client=0x5637edc888f0,
msg=<optimized out>, rerr=0x7f057ba6cc90, args=0x7f054c0008c0,
ret=0x7f054c0008e0) at remote_dispatch.h:424
#3 0x00005637ec6538a8 in virNetServerProgramDispatchCall
(msg=0x5637edc89d40, client=0x5637edc888f0, server=0x5637edc6bf10,
prog=0x5637edc84810) at rpc/virnetserverprogram.c:474
#4 virNetServerProgramDispatch (prog=0x5637edc84810,
server=server@entry=0x5637edc6bf10, client=0x5637edc888f0,
msg=0x5637edc89d40) at rpc/virnetserverprogram.c:311
#5 0x00005637ec64f44d in virNetServerProcessMsg (msg=<optimized out>,
prog=<optimized out>, client=<optimized out>,
srv=0x5637edc6bf10) at rpc/virnetserver.c:148
#6 virNetServerHandleJob (jobOpaque=<optimized out>,
opaque=0x5637edc6bf10) at rpc/virnetserver.c:169
#7 0x00007f058de6c311 in virThreadPoolWorker
(opaque=opaque@entry=0x5637edc603e0) at util/virthreadpool.c:167
#8 0x00007f058de6b628 in virThreadHelper (data=<optimized out>) at
util/virthread.c:219
#9 0x00007f058ae55dc5 in start_thread () from /usr/lib64/libpthread.so.0
#10 0x00007f058ab836fd in clone () from /usr/lib64/libc.so.6
Old value = 5
New value = 4
virObjectUnref (anyobj=0x5637edc88630) at util/virobject.c:259
259 PROBE(OBJECT_UNREF, "obj=%p", obj);
Continuing.
[Switching to Thread 0x7f058eb1f840 (LWP 3878)]
Hardware watchpoint 9: *(int *) 0x5637edc88634
Old value = 4
New value = 5
virObjectRef (anyobj=anyobj@entry=0x5637edc88630) at util/virobject.c:296
296 PROBE(OBJECT_REF, "obj=%p", obj);
#0 virObjectRef (anyobj=anyobj@entry=0x5637edc88630) at
util/virobject.c:296
#1 0x00005637ec65299d in virNetServerClientClose
(client=0x5637edc88630) at rpc/virnetserverclient.c:978
#2 0x00005637ec650728 in virNetServerProcessClients
(srv=0x5637edc6bf10) at rpc/virnetserver.c:861
#3 0x00007f058df8b099 in daemonServerProcessClients (payload=<optimized
out>, key=<optimized out>, opaque=<optimized out>)
at rpc/virnetdaemon.c:764
#4 0x00007f058de1921d in virHashForEach (table=0x5637edc6d950,
iter=iter@entry=0x7f058df8b090 <daemonServerProcessClients>,
data=data@entry=0x0) at util/virhash.c:606
#5 0x00007f058df8c14f in virNetDaemonRun (dmn=0x5637edc6f270) at
rpc/virnetdaemon.c:831
#6 0x00005637ec610c48 in main (argc=<optimized out>, argv=<optimized
out>) at libvirtd.c:1605
Continuing.
Hardware watchpoint 9: *(int *) 0x5637edc88634
Old value = 5
New value = 4
virObjectUnref (anyobj=anyobj@entry=0x5637edc88630) at util/virobject.c:259
259 PROBE(OBJECT_UNREF, "obj=%p", obj);
#0 virObjectUnref (anyobj=anyobj@entry=0x5637edc88630) at
util/virobject.c:259
#1 0x00005637ec6529bd in virNetServerClientClose
(client=0x5637edc88630) at rpc/virnetserverclient.c:982
#2 0x00005637ec650728 in virNetServerProcessClients
(srv=0x5637edc6bf10) at rpc/virnetserver.c:861
#3 0x00007f058df8b099 in daemonServerProcessClients (payload=<optimized
out>, key=<optimized out>, opaque=<optimized out>)
at rpc/virnetdaemon.c:764
#4 0x00007f058de1921d in virHashForEach (table=0x5637edc6d950,
iter=iter@entry=0x7f058df8b090 <daemonServerProcessClients>,
data=data@entry=0x0) at util/virhash.c:606
#5 0x00007f058df8c14f in virNetDaemonRun (dmn=0x5637edc6f270) at
rpc/virnetdaemon.c:831
#6 0x00005637ec610c48 in main (argc=<optimized out>, argv=<optimized
out>) at libvirtd.c:1605
Continuing.
Hardware watchpoint 9: *(int *) 0x5637edc88634
Old value = 4
New value = 5
virObjectRef (anyobj=anyobj@entry=0x5637edc88630) at util/virobject.c:296
296 PROBE(OBJECT_REF, "obj=%p", obj);
#0 virObjectRef (anyobj=anyobj@entry=0x5637edc88630) at
util/virobject.c:296
#1 0x00005637ec6529d1 in virNetServerClientClose
(client=0x5637edc88630) at rpc/virnetserverclient.c:987
#2 0x00005637ec650728 in virNetServerProcessClients
(srv=0x5637edc6bf10) at rpc/virnetserver.c:861
#3 0x00007f058df8b099 in daemonServerProcessClients (payload=<optimized
out>, key=<optimized out>, opaque=<optimized out>)
at rpc/virnetdaemon.c:764
#4 0x00007f058de1921d in virHashForEach (table=0x5637edc6d950,
iter=iter@entry=0x7f058df8b090 <daemonServerProcessClients>,
data=data@entry=0x0) at util/virhash.c:606
#5 0x00007f058df8c14f in virNetDaemonRun (dmn=0x5637edc6f270) at
rpc/virnetdaemon.c:831
#6 0x00005637ec610c48 in main (argc=<optimized out>, argv=<optimized
out>) at libvirtd.c:1605
Continuing.
Hardware watchpoint 9: *(int *) 0x5637edc88634
Old value = 5
New value = 4
virObjectUnref (anyobj=anyobj@entry=0x5637edc88630) at util/virobject.c:259
259 PROBE(OBJECT_UNREF, "obj=%p", obj);
#0 virObjectUnref (anyobj=anyobj@entry=0x5637edc88630) at
util/virobject.c:259
#1 0x00005637ec6529ee in virNetServerClientClose
(client=0x5637edc88630) at rpc/virnetserverclient.c:991
#2 0x00005637ec650728 in virNetServerProcessClients
(srv=0x5637edc6bf10) at rpc/virnetserver.c:861
#3 0x00007f058df8b099 in daemonServerProcessClients (payload=<optimized
out>, key=<optimized out>, opaque=<optimized out>)
at rpc/virnetdaemon.c:764
#4 0x00007f058de1921d in virHashForEach (table=0x5637edc6d950,
iter=iter@entry=0x7f058df8b090 <daemonServerProcessClients>,
data=data@entry=0x0) at util/virhash.c:606
#5 0x00007f058df8c14f in virNetDaemonRun (dmn=0x5637edc6f270) at
rpc/virnetdaemon.c:831
#6 0x00005637ec610c48 in main (argc=<optimized out>, argv=<optimized
out>) at libvirtd.c:1605
Continuing.
Hardware watchpoint 9: *(int *) 0x5637edc88634
Old value = 4
New value = 3
virObjectUnref (anyobj=anyobj@entry=0x5637edc88630) at util/virobject.c:259
259 PROBE(OBJECT_UNREF, "obj=%p", obj);
#0 virObjectUnref (anyobj=anyobj@entry=0x5637edc88630) at
util/virobject.c:259
#1 0x00005637ec650705 in virNetServerProcessClients
(srv=0x5637edc6bf10) at rpc/virnetserver.c:873
#2 0x00007f058df8b099 in daemonServerProcessClients (payload=<optimized
out>, key=<optimized out>, opaque=<optimized out>)
at rpc/virnetdaemon.c:764
#3 0x00007f058de1921d in virHashForEach (table=0x5637edc6d950,
iter=iter@entry=0x7f058df8b090 <daemonServerProcessClients>,
data=data@entry=0x0) at util/virhash.c:606
#4 0x00007f058df8c14f in virNetDaemonRun (dmn=0x5637edc6f270) at
rpc/virnetdaemon.c:831
#5 0x00005637ec610c48 in main (argc=<optimized out>, argv=<optimized
out>) at libvirtd.c:1605
Continuing.
Hardware watchpoint 9: *(int *) 0x5637edc88634
Old value = 3
New value = 2
virObjectUnref (anyobj=0x5637edc88630) at util/virobject.c:259
259 PROBE(OBJECT_UNREF, "obj=%p", obj);
#0 virObjectUnref (anyobj=0x5637edc88630) at util/virobject.c:259
#1 0x00007f058de4199b in virObjectUnref (anyobj=<optimized out>) at
util/virobject.c:265
#2 0x00007f058de0db34 in virEventPollCleanupTimeouts () at
util/vireventpoll.c:543
#3 0x00007f058de0ea7e in virEventPollRunOnce () at util/vireventpoll.c:628
#4 0x00007f058de0d892 in virEventRunDefaultImpl () at util/virevent.c:314
#5 0x00007f058df8c12d in virNetDaemonRun (dmn=0x5637edc6f270) at
rpc/virnetdaemon.c:824
#6 0x00005637ec610c48 in main (argc=<optimized out>, argv=<optimized
out>) at libvirtd.c:1605
Continuing.
Hardware watchpoint 9: *(int *) 0x5637edc88634
Old value = 2
New value = 1
virObjectUnref (anyobj=0x5637edc88630) at util/virobject.c:259
259 PROBE(OBJECT_UNREF, "obj=%p", obj);
#0 virObjectUnref (anyobj=0x5637edc88630) at util/virobject.c:259
#1 0x00005637ec656bee in virNetSocketEventFree
(opaque=opaque@entry=0x5637edc8a5b0) at rpc/virnetsocket.c:2152
#2 0x00007f058de0dd34 in virEventPollCleanupHandles () at
util/vireventpoll.c:592
#3 0x00007f058de0ea83 in virEventPollRunOnce () at util/vireventpoll.c:629
#4 0x00007f058de0d892 in virEventRunDefaultImpl () at util/virevent.c:314
#5 0x00007f058df8c12d in virNetDaemonRun (dmn=0x5637edc6f270) at
rpc/virnetdaemon.c:824
#6 0x00005637ec610c48 in main (argc=<optimized out>, argv=<optimized
out>) at libvirtd.c:1605
Continuing.
so the client refs alway have 1 when I register one event callbacks.
> this patch will deRegister all event callbacks when client was
close.
> ---
FWIW: This commit message was a bit difficult to follow - I know it's
primarily a language barrier thing - so maybe if you just construct some
function code flow for both initialization/open and destruction/close
it'd help... Paths that work and the path that you think is broken...
That can be done in this section after the "---" and before the changed
API's.
I assume you've done a lot of research, but the details of that research
can be difficult to just "pick up on". Typically someone will provide
some sort of valgrind output as an example.
I just have a sense that this could boil down to a path (or two) that
are doing the virObjectRef without the expected virObjectUnref occurring
- something perhaps to do with how @wantClose is handled in the abnormal
termination case. Nothing jumps out at me though even after looking at
the code for a while.
> daemon/remote.c | 46 ++++++++++++++++++++++++++++------------------
> 1 file changed, 28 insertions(+), 18 deletions(-)
>
> diff --git a/daemon/remote.c b/daemon/remote.c
> index cbcb6e8..466b2e7 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -1689,23 +1689,10 @@ void remoteRelayConnectionClosedEvent(virConnectPtr conn
ATTRIBUTE_UNUSED, int r
> 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
> +remoteClientFreePrivateCallbacks(struct daemonClientPrivate *priv)
> {
> - struct daemonClientPrivate *priv = data;
> -
> - /* Deregister event delivery callback */
> - if (priv->conn) {
> - virIdentityPtr sysident = virIdentityGetSystem();
> -
> - virIdentitySetCurrent(sysident);
> -
> + if (priv && priv->conn) {
Well each of the callers has already dereferenced @priv so it's a bit
too late to check it now!
yes, I also think this check is too late. this placement
can only check
priv->conn.
> DEREG_CB(priv->conn, priv->domainEventCallbacks,
> priv->ndomainEventCallbacks,
> virConnectDomainEventDeregisterAny, "domain");
> @@ -1730,16 +1717,38 @@ void remoteClientFreeFunc(void *data)
> remoteRelayConnectionClosedEvent)
< 0)
> VIR_WARN("unexpected close callback event deregister
failure");
> }
> + }
> +}
> +#undef DEREG_CB
> +
There should be another blank line here.
OK, I lost a blank line.
> +/*
> + * 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;
> +
> + /* Deregister event delivery callback */
> + if (priv->conn) {
> + virIdentityPtr sysident = virIdentityGetSystem();
> +
> + virIdentitySetCurrent(sysident);
> +
> + remoteClientFreePrivateCallbacks(priv);
>
> virConnectClose(priv->conn);
>
> virIdentitySetCurrent(NULL);
> virObjectUnref(sysident);
> - }
>
> + }
> VIR_FREE(priv);
> +
> }
> -#undef DEREG_CB
>
>
> static void remoteClientCloseFunc(virNetServerClientPtr client)
> @@ -1747,6 +1756,7 @@ static void remoteClientCloseFunc(virNetServerClientPtr
client)
> struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
>
> daemonRemoveAllClientStreams(priv->streams);
> + remoteClientFreePrivateCallbacks(priv);
Perhaps this just gets a "if (priv->conn)" before calling...
Still if as I assume the missing Unref is found (someday), then
theoretically remoteClientFreeFunc would be called - of course the
nEventCallbacks = 0 will ensure the Dereg's dont' get called again. So
it shoudn't be a problem
In my opinion, it is safe to check point is Null. But
check everywhere
is stupid.
so it is hard to determine where need check.:-)
John
> }
>
>
>
.