[libvirt] [PATCH v2] Ensure client streams are closed when marking a client for close

[Daniel P. Berrange]

From: "Daniel P. Berrange" <berrange at redhat.com>

NB, previous patch was borked due to bad rebase

Every active stream results in a reference being held on the
virNetServerClientPtr object. This meant that if a client quit
with any streams active, although all I/O was stopped the
virNetServerClientPtr object would leak. This causes libvirtd
to leak any file handles associated with open streams when a
client quit

To fix this, when we call virNetServerClientClose there is a
callback invoked which lets the daemon release the streams
and thus the extra references

* daemon/remote.c: Add a hook to close all streams
* daemon/stream.c, daemon/stream.h: Add API for releasing
  all streams
* src/rpc/virnetserverclient.c, src/rpc/virnetserverclient.h:
  Allow registration of a hook to trigger when closing client
---
 daemon/remote.c              |   11 ++++++++++-
 daemon/stream.c              |   38 ++++++++++++++++++++++++++++++++------
 daemon/stream.h              |    3 +++
 src/rpc/virnetserverclient.c |   21 +++++++++++++++++++++
 src/rpc/virnetserverclient.h |    5 +++++
 5 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index 939044c..0f088c6 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -439,6 +439,13 @@ static void remoteClientFreeFunc(void *data)
 }
 
 
+static void remoteClientCloseFunc(virNetServerClientPtr client)
+{
+    struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
+
+    daemonRemoveAllClientStreams(priv->streams);
+}
+
 
 int remoteClientInitHook(virNetServerPtr srv ATTRIBUTE_UNUSED,
                          virNetServerClientPtr client)
@@ -460,7 +467,9 @@ int remoteClientInitHook(virNetServerPtr srv ATTRIBUTE_UNUSED,
     for (i = 0 ; i < VIR_DOMAIN_EVENT_ID_LAST ; i++)
         priv->domainEventCallbackID[i] = -1;
 
-    virNetServerClientSetPrivateData(client, priv, remoteClientFreeFunc);
+    virNetServerClientSetPrivateData(client, priv,
+                                     remoteClientFreeFunc);
+    virNetServerClientSetCloseHook(client, remoteClientCloseFunc);
     return 0;
 }
 
diff --git a/daemon/stream.c b/daemon/stream.c
index 4a8f1ee..c6865a8 100644
--- a/daemon/stream.c
+++ b/daemon/stream.c
@@ -334,13 +334,17 @@ int daemonFreeClientStream(virNetServerClientPtr client,
     msg = stream->rx;
     while (msg) {
         virNetMessagePtr tmp = msg->next;
-        /* Send a dummy reply to free up 'msg' & unblock client rx */
-        memset(msg, 0, sizeof(*msg));
-        msg->header.type = VIR_NET_REPLY;
-        if (virNetServerClientSendMessage(client, msg) < 0) {
-            virNetServerClientImmediateClose(client);
+        if (client) {
+            /* Send a dummy reply to free up 'msg' & unblock client rx */
+            memset(msg, 0, sizeof(*msg));
+            msg->header.type = VIR_NET_REPLY;
+            if (virNetServerClientSendMessage(client, msg) < 0) {
+                virNetServerClientImmediateClose(client);
+                virNetMessageFree(msg);
+                ret = -1;
+            }
+        } else {
             virNetMessageFree(msg);
-            ret = -1;
         }
         msg = tmp;
     }
@@ -441,6 +445,28 @@ daemonRemoveClientStream(virNetServerClientPtr client,
 }
 
 
+void
+daemonRemoveAllClientStreams(daemonClientStream *stream)
+{
+    daemonClientStream *tmp;
+
+    VIR_DEBUG("stream=%p", stream);
+
+    while (stream) {
+        tmp = stream->next;
+
+        if (!stream->closed) {
+            virStreamEventRemoveCallback(stream->st);
+            virStreamAbort(stream->st);
+        }
+
+        daemonFreeClientStream(NULL, stream);
+
+        VIR_DEBUG("next stream=%p", stream);
+        stream = tmp;
+    }
+}
+
 /*
  * Returns:
  *   -1  if fatal error occurred
diff --git a/daemon/stream.h b/daemon/stream.h
index 626ae60..7c2d8dc 100644
--- a/daemon/stream.h
+++ b/daemon/stream.h
@@ -45,4 +45,7 @@ int
 daemonRemoveClientStream(virNetServerClientPtr client,
                          daemonClientStream *stream);
 
+void
+daemonRemoveAllClientStreams(daemonClientStream *stream);
+
 #endif /* __LIBVIRTD_STREAM_H__ */
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index e246fa5..a73b06d 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -97,6 +97,7 @@ struct _virNetServerClient
 
     void *privateData;
     virNetServerClientFreeFunc privateDataFreeFunc;
+    virNetServerClientCloseFunc privateDataCloseFunc;
 };
 
 
@@ -492,6 +493,15 @@ void *virNetServerClientGetPrivateData(virNetServerClientPtr client)
 }
 
 
+void virNetServerClientSetCloseHook(virNetServerClientPtr client,
+                                    virNetServerClientCloseFunc cf)
+{
+    virNetServerClientLock(client);
+    client->privateDataCloseFunc = cf;
+    virNetServerClientUnlock(client);
+}
+
+
 void virNetServerClientSetDispatcher(virNetServerClientPtr client,
                                      virNetServerClientDispatchFunc func,
                                      void *opaque)
@@ -560,6 +570,8 @@ void virNetServerClientFree(virNetServerClientPtr client)
  */
 void virNetServerClientClose(virNetServerClientPtr client)
 {
+    virNetServerClientCloseFunc cf;
+
     virNetServerClientLock(client);
     VIR_DEBUG("client=%p refs=%d", client, client->refs);
     if (!client->sock) {
@@ -567,6 +579,15 @@ void virNetServerClientClose(virNetServerClientPtr client)
         return;
     }
 
+    if (client->privateDataCloseFunc) {
+        cf = client->privateDataCloseFunc;
+        client->refs++;
+        virNetServerClientUnlock(client);
+        (cf)(client);
+        virNetServerClientLock(client);
+        client->refs--;
+    }
+
     /* Do now, even though we don't close the socket
      * until end, to ensure we don't get invoked
      * again due to tls shutdown */
diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
index 3d2e1fb..bedb179 100644
--- a/src/rpc/virnetserverclient.h
+++ b/src/rpc/virnetserverclient.h
@@ -82,6 +82,11 @@ void virNetServerClientSetPrivateData(virNetServerClientPtr client,
                                       virNetServerClientFreeFunc ff);
 void *virNetServerClientGetPrivateData(virNetServerClientPtr client);
 
+typedef void (*virNetServerClientCloseFunc)(virNetServerClientPtr client);
+
+void virNetServerClientSetCloseHook(virNetServerClientPtr client,
+                                    virNetServerClientCloseFunc cf);
+
 void virNetServerClientSetDispatcher(virNetServerClientPtr client,
                                      virNetServerClientDispatchFunc func,
                                      void *opaque);
-- 
1.7.6