I supplied a bit more details w/r/t what I found for REF/UNREF - I
assume it essentially matches what you've found. But having it here
certainly helps - so thanks for providing it along with the example.
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
Although the line numbers are different with top of tree, this is the
RegisterAny REF on the client...
[...]
So that says to me we generally run with 3-4 REFs (without any Register
Event occurring). AFAICT, the following is the list:
1. Adding to the srv->clients (virNetServerAddClient)
2. Adding IO Callback (virNetServerClientRegisterEvent)
3. Adding Keep Alive (virNetServerClientInitKeepAlive and
virKeepAliveStart)
The 4th is a bit trickier, but I believe it's REF'd during the call to
virNetServerClientDispatchRead...
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
I'm assuming this comes from the opposite end of the DispatchRead -
there's no trace here to prove it, but that's not necessarily surprising
either as it's not easy to find where the Unref would naturally occur
without an error path occurring....
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
[...]
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
This pair of REF/UNREF in virNetServerClientClose when client->keepalive
is set.
[...]
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
[...]
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
This pair of REF/UNREF in virNetServerClientClose when
client->privateDataCloseFunc is set.
[...]
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
Again my line numbers are off a bit, but this would be the for the UNREF
after VIR_DELETE_ELEMENT when removed from srv->clients
[...]
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
[...]
This one is the opposite end of the virObjectRef done for the keep alive
timer. See virNetServerClientInitKeepAlive and virKeepAliveStart usage
of virEventAddTimeout.
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
[...]
This one is the opposite end of the virObjectRef done in
virNetServerClientRegisterEvent prior to virNetSocketAddIOCallback which
calls virEventAddHandle.
Theoretically the "last" REF...
John
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
}
.
.
