[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