[libvirt] [PATCH v2 2/2] remote: free client all event callbacks at remoteClientCloseFunc
xinhua.Cao
caoxinhua at huawei.com
Sat Nov 11 08:20:53 UTC 2017
在 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 at entry=0x5637edc888f0) at util/virobject.c:296
296 PROBE(OBJECT_REF, "obj=%p", obj);
(gdb) bt
#0 virObjectRef (anyobj=anyobj at 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 at 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 at 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 at entry=0x5637edc88630) at util/virobject.c:296
296 PROBE(OBJECT_REF, "obj=%p", obj);
#0 virObjectRef (anyobj=anyobj at 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 at entry=0x7f058df8b090 <daemonServerProcessClients>,
data=data at 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 at entry=0x5637edc88630) at util/virobject.c:259
259 PROBE(OBJECT_UNREF, "obj=%p", obj);
#0 virObjectUnref (anyobj=anyobj at 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 at entry=0x7f058df8b090 <daemonServerProcessClients>,
data=data at 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 at entry=0x5637edc88630) at util/virobject.c:296
296 PROBE(OBJECT_REF, "obj=%p", obj);
#0 virObjectRef (anyobj=anyobj at 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 at entry=0x7f058df8b090 <daemonServerProcessClients>,
data=data at 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 at entry=0x5637edc88630) at util/virobject.c:259
259 PROBE(OBJECT_UNREF, "obj=%p", obj);
#0 virObjectUnref (anyobj=anyobj at 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 at entry=0x7f058df8b090 <daemonServerProcessClients>,
data=data at 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 at entry=0x5637edc88630) at util/virobject.c:259
259 PROBE(OBJECT_UNREF, "obj=%p", obj);
#0 virObjectUnref (anyobj=anyobj at 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 at entry=0x7f058df8b090 <daemonServerProcessClients>,
data=data at 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 at 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
>
>> }
>>
>>
>>
> .
>
More information about the libvir-list
mailing list