8:10 a.m.
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
8:10 a.m.
New subject: [libvirt] [PATCH 1/4] daemonSetupNetworking: Don't leak services
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(a)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
8:11 a.m.
New subject: [libvirt] [PATCH 2/4] virNetSocketRemoveIOCallback: Be explicit about unref
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(a)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
4:09 a.m.
New subject: [libvirt] [PATCH 2/4] virNetSocketRemoveIOCallback: Be explicit about unref
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(a)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
8:11 a.m.
New subject: [libvirt] [PATCH 3/4] virNetSocket: Fix @watch corner case
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(a)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
8:11 a.m.
New subject: [libvirt] [PATCH 4/4] virNetServerServiceClose: Don't leak sockets
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(a)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
4:10 a.m.
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
3444
days inactive
3445
days old
6 comments
2 participants
participants (2)
-
Erik Skultety -
Michal Privoznik