[libvirt] [PATCH 0/4] A cople of memleak fixes
One thing that I still haven't figured out is, when the daemon is waiting in the even loop, in poll() and SIGINT comes, it jumps out and cleanup phase begins. However, the event loop may still be the one and only holding a reference to some objects, so we kind of want it to at least remove stale handlers. Well, they will be deleted after the cleanup work in the daemon is done. Yeah, messy and probably not worth breaking up the code into smaller pieces just to cleanup deleted handles. Michal Privoznik (4): daemonSetupNetworking: Don't leak services virNetSocketRemoveIOCallback: Be explicit about unref virNetSocket: Fix @watch corner case virNetServerServiceClose: Don't leak sockets daemon/libvirtd.c | 47 ++++++++++++++++++++++--------------------- src/rpc/virnetserverservice.c | 5 ++++- src/rpc/virnetsocket.c | 11 ++++++---- 3 files changed, 35 insertions(+), 28 deletions(-) -- 2.3.6
When setting up the daemon networking, new services are created. These services then have sockets to listen on. Once created, the service objects are added to corresponding server object. However, during that process, server increases reference counter of the service object. So, at the end of the function, we should decrease it again. This way the service objects will have only 1 reference, but that's okay since servers are the only objects having a reference. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/libvirtd.c | 47 ++++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 286512a..20e0b2f 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -455,33 +455,34 @@ daemonSetupNetworking(virNetServerPtr srv, int unix_sock_ro_mask = 0; int unix_sock_rw_mask = 0; int unix_sock_adm_mask = 0; + int ret = -1; unsigned int cur_fd = STDERR_FILENO + 1; unsigned int nfds = virGetListenFDs(); if (config->unix_sock_group) { if (virGetGroupID(config->unix_sock_group, &unix_sock_gid) < 0) - return -1; + return ret; } if (nfds > (sock_path_ro ? 2 : 1)) { VIR_ERROR(_("Too many (%u) FDs passed from caller"), nfds); - return -1; + return ret; } if (virStrToLong_i(config->unix_sock_ro_perms, NULL, 8, &unix_sock_ro_mask) != 0) { VIR_ERROR(_("Failed to parse mode '%s'"), config->unix_sock_ro_perms); - goto error; + goto cleanup; } if (virStrToLong_i(config->unix_sock_admin_perms, NULL, 8, &unix_sock_adm_mask) != 0) { VIR_ERROR(_("Failed to parse mode '%s'"), config->unix_sock_admin_perms); - goto error; + goto cleanup; } if (virStrToLong_i(config->unix_sock_rw_perms, NULL, 8, &unix_sock_rw_mask) != 0) { VIR_ERROR(_("Failed to parse mode '%s'"), config->unix_sock_rw_perms); - goto error; + goto cleanup; } if (!(svc = virNetServerServiceNewFDOrUNIX(sock_path, @@ -495,7 +496,7 @@ daemonSetupNetworking(virNetServerPtr srv, config->max_queued_clients, config->max_client_requests, nfds, &cur_fd))) - goto error; + goto cleanup; if (sock_path_ro) { if (!(svcRO = virNetServerServiceNewFDOrUNIX(sock_path_ro, unix_sock_ro_mask, @@ -508,18 +509,18 @@ daemonSetupNetworking(virNetServerPtr srv, config->max_queued_clients, config->max_client_requests, nfds, &cur_fd))) - goto error; + goto cleanup; } if (virNetServerAddService(srv, svc, config->mdns_adv && !ipsock ? "_libvirt._tcp" : NULL) < 0) - goto error; + goto cleanup; if (svcRO && virNetServerAddService(srv, svcRO, NULL) < 0) - goto error; + goto cleanup; if (sock_path_adm) { VIR_DEBUG("Registering unix socket %s", sock_path_adm); @@ -533,10 +534,10 @@ daemonSetupNetworking(virNetServerPtr srv, true, config->admin_max_queued_clients, config->admin_max_client_requests))) - goto error; + goto cleanup; if (virNetServerAddService(srvAdm, svcAdm, NULL) < 0) - goto error; + goto cleanup; } if (ipsock) { @@ -553,11 +554,11 @@ daemonSetupNetworking(virNetServerPtr srv, false, config->max_queued_clients, config->max_client_requests))) - goto error; + goto cleanup; if (virNetServerAddService(srv, svcTCP, config->mdns_adv ? "_libvirt._tcp" : NULL) < 0) - goto error; + goto cleanup; } #if WITH_GNUTLS @@ -574,14 +575,14 @@ daemonSetupNetworking(virNetServerPtr srv, (const char *const*)config->tls_allowed_dn_list, config->tls_no_sanity_certificate ? false : true, config->tls_no_verify_certificate ? false : true))) - goto error; + goto cleanup; } else { if (!(ctxt = virNetTLSContextNewServerPath(NULL, !privileged, (const char *const*)config->tls_allowed_dn_list, config->tls_no_sanity_certificate ? false : true, config->tls_no_verify_certificate ? false : true))) - goto error; + goto cleanup; } VIR_DEBUG("Registering TLS socket %s:%s", @@ -596,12 +597,12 @@ daemonSetupNetworking(virNetServerPtr srv, config->max_queued_clients, config->max_client_requests))) { virObjectUnref(ctxt); - goto error; + goto cleanup; } if (virNetServerAddService(srv, svcTLS, config->mdns_adv && !config->listen_tcp ? "_libvirt._tcp" : NULL) < 0) - goto error; + goto cleanup; virObjectUnref(ctxt); } @@ -610,7 +611,7 @@ daemonSetupNetworking(virNetServerPtr srv, if (config->listen_tls) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("This libvirtd build does not support TLS")); - goto error; + goto cleanup; } #endif } @@ -625,21 +626,21 @@ daemonSetupNetworking(virNetServerPtr srv, saslCtxt = virNetSASLContextNewServer( (const char *const*)config->sasl_allowed_username_list); if (!saslCtxt) - goto error; + goto cleanup; } #endif - return 0; + ret = 0; - error: + cleanup: #if WITH_GNUTLS virObjectUnref(svcTLS); #endif virObjectUnref(svcTCP); - virObjectUnref(svc); virObjectUnref(svcRO); virObjectUnref(svcAdm); - return -1; + virObjectUnref(svc); + return ret; } -- 2.3.6
When going through the code I've notice that virNetSocketAddIOCallback() increases the reference counter of @socket. However, its counter part RemoveIOCallback does not. It took me a while to realize this disproportion. The AddIOCallback registers our own callback which eventually calls the desired callback and then unref the @sock. Yeah, a bit complicated but it works. So, lets note this hard learned fact in a comment in RemoveIOCallback(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetsocket.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 2497f67..81903e7 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -1993,6 +1993,7 @@ void virNetSocketRemoveIOCallback(virNetSocketPtr sock) } virEventRemoveHandle(sock->watch); + /* Don't unref @sock, it's done via evenloop callback. */ virObjectUnlock(sock); } -- 2.3.6
On 06/18/2015 03:11 PM, Michal Privoznik wrote:
When going through the code I've notice that virNetSocketAddIOCallback() increases the reference counter of @socket. However, its counter part RemoveIOCallback does not. It took me a while to realize this disproportion. The AddIOCallback registers our own callback which eventually calls the desired callback and then unref the @sock. Yeah, a bit complicated but it works. So, lets note this hard learned fact in a comment in RemoveIOCallback().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetsocket.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 2497f67..81903e7 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -1993,6 +1993,7 @@ void virNetSocketRemoveIOCallback(virNetSocketPtr sock) }
virEventRemoveHandle(sock->watch); + /* Don't unref @sock, it's done via evenloop callback. */
s/evenloop/eventloop Since this is a note/reminder you could mark this one as NOTE.
virObjectUnlock(sock); }
ACK with that minor adjustment. Erik
Although highly unlikely, nobody says that virEventAddHandle() can't return 0 as a handle to socket callback. It can't happen with our default implementation since all watches will have value 1 or greater, but users can register their own callback functions (which can re-use unused watch IDs for instance). If this is the case, weird things may happen. Also, there's a little bug I'm fixing too, upon virNetSocketRemoveIOCallback(), the variable holding callback ID was not reset. Therefore calling AddIOCallback() once again would fail. Not that we are doing it right now, but we might. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetsocket.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 81903e7..cef3f36 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -245,6 +245,7 @@ static virNetSocketPtr virNetSocketNew(virSocketAddrPtr localAddr, sock->fd = fd; sock->errfd = errfd; sock->pid = pid; + sock->watch = -1; /* Disable nagle for TCP sockets */ if (sock->localAddr.data.sa.sa_family == AF_INET || @@ -1153,7 +1154,7 @@ void virNetSocketDispose(void *obj) PROBE(RPC_SOCKET_DISPOSE, "sock=%p", sock); - if (sock->watch > 0) { + if (sock->watch >= 0) { virEventRemoveHandle(sock->watch); sock->watch = -1; } @@ -1941,7 +1942,7 @@ int virNetSocketAddIOCallback(virNetSocketPtr sock, virObjectRef(sock); virObjectLock(sock); - if (sock->watch > 0) { + if (sock->watch >= 0) { VIR_DEBUG("Watch already registered on socket %p", sock); goto cleanup; } @@ -1971,7 +1972,7 @@ void virNetSocketUpdateIOCallback(virNetSocketPtr sock, int events) { virObjectLock(sock); - if (sock->watch <= 0) { + if (sock->watch < 0) { VIR_DEBUG("Watch not registered on socket %p", sock); virObjectUnlock(sock); return; @@ -1986,7 +1987,7 @@ void virNetSocketRemoveIOCallback(virNetSocketPtr sock) { virObjectLock(sock); - if (sock->watch <= 0) { + if (sock->watch < 0) { VIR_DEBUG("Watch not registered on socket %p", sock); virObjectUnlock(sock); return; @@ -1994,6 +1995,7 @@ void virNetSocketRemoveIOCallback(virNetSocketPtr sock) virEventRemoveHandle(sock->watch); /* Don't unref @sock, it's done via evenloop callback. */ + sock->watch = -1; virObjectUnlock(sock); } -- 2.3.6
Well, if a server is being destructed, all underlying services and their sockets should disappear with it. But due to bug in our implementation this is not the case. Yes, we are closing the sockets, but that's not enough. We must also: 1) Unregister them from the event loop 2) Unref the service for each socket The last step is needed, because each socket callback holds a reference to the service object. Since in the first step we are unregistering the callbacks, they no longer need the reference. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetserverservice.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 4df26cb..3b35fc0 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -520,6 +520,9 @@ void virNetServerServiceClose(virNetServerServicePtr svc) if (!svc) return; - for (i = 0; i < svc->nsocks; i++) + for (i = 0; i < svc->nsocks; i++) { + virNetSocketRemoveIOCallback(svc->socks[i]); virNetSocketClose(svc->socks[i]); + virObjectUnref(svc); + } } -- 2.3.6
On 06/18/2015 03:10 PM, Michal Privoznik wrote:
One thing that I still haven't figured out is, when the daemon is waiting in the even loop, in poll() and SIGINT comes, it jumps out and cleanup phase begins. However, the event loop may still be the one and only holding a reference to some objects, so we kind of want it to at least remove stale handlers. Well, they will be deleted after the cleanup work in the daemon is done. Yeah, messy and probably not worth breaking up the code into smaller pieces just to cleanup deleted handles.
Michal Privoznik (4): daemonSetupNetworking: Don't leak services virNetSocketRemoveIOCallback: Be explicit about unref virNetSocket: Fix @watch corner case virNetServerServiceClose: Don't leak sockets
daemon/libvirtd.c | 47 ++++++++++++++++++++++--------------------- src/rpc/virnetserverservice.c | 5 ++++- src/rpc/virnetsocket.c | 11 ++++++---- 3 files changed, 35 insertions(+), 28 deletions(-)
ACK series, see 2/4 for that minor adjustment. Erik
participants (2)
-
Erik Skultety -
Michal Privoznik