[PATCH 07/14] rpc: Correct locking and simplify the function

[Marc Hartmayer <mhartmay@xxxxxxxxxxxxxxxxxx>]

The lock for @client must not only be held for the duration of
checking whether the client wants to close, but also for as long as
we're closing the client.

Signed-off-by: Marc Hartmayer <mhartmay@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Stefan Zimmermann <stzi@xxxxxxxxxxxxxxxxxx>
---
 src/libvirt_remote.syms      |  3 ++-
 src/rpc/virnetserver.c       | 12 ++++++++----
 src/rpc/virnetserverclient.c | 25 ++++++++++++++-----------
 src/rpc/virnetserverclient.h |  3 ++-
 4 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 61c20d530bc8..3ce5694b781d 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -125,6 +125,7 @@ virNetServerUpdateServices;
 # rpc/virnetserverclient.h
 virNetServerClientAddFilter;
 virNetServerClientClose;
+virNetServerClientCloseLocked;
 virNetServerClientDelayedClose;
 virNetServerClientGetAuth;
 virNetServerClientGetFD;
@@ -154,7 +155,7 @@ virNetServerClientSetAuth;
 virNetServerClientSetCloseHook;
 virNetServerClientSetDispatcher;
 virNetServerClientStartKeepAlive;
-virNetServerClientWantClose;
+virNetServerClientWantCloseLocked;
 
 
 # rpc/virnetservermdns.h
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 2b76daab5566..dde9b73fc250 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -846,6 +846,7 @@ void
 virNetServerProcessClients(virNetServerPtr srv)
 {
     size_t i;
+    virNetServerClientPtr client;
 
     virObjectLock(srv);
 
@@ -854,11 +855,14 @@ virNetServerProcessClients(virNetServerPtr srv)
         /* Coverity 5.3.0 couldn't see that srv->clients is non-NULL
          * if srv->nclients is non-zero.  */
         sa_assert(srv->clients);
-        if (virNetServerClientWantClose(srv->clients[i]))
-            virNetServerClientClose(srv->clients[i]);
-        if (virNetServerClientIsClosed(srv->clients[i])) {
-            virNetServerClientPtr client = srv->clients[i];
 
+        client = srv->clients[i];
+        virObjectLock(client);
+        if (virNetServerClientWantCloseLocked(client))
+            virNetServerClientCloseLocked(client);
+        virObjectUnlock(client);
+
+        if (virNetServerClientIsClosed(client)) {
             VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients);
 
             if (virNetServerClientNeedAuth(client))
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 0ee299e2d6ec..96fd1e6d15c2 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -962,17 +962,15 @@ void virNetServerClientDispose(void *obj)
  * Full free of the client is done later in a safe point
  * where it can be guaranteed it is no longer in use
  */
-void virNetServerClientClose(virNetServerClientPtr client)
+void
+virNetServerClientCloseLocked(virNetServerClientPtr client)
 {
     virNetServerClientCloseFunc cf;
     virKeepAlivePtr ka;
 
-    virObjectLock(client);
     VIR_DEBUG("client=%p", client);
-    if (!client->sock) {
-        virObjectUnlock(client);
+    if (!client->sock)
         return;
-    }
 
     if (client->keepalive) {
         virKeepAliveStop(client->keepalive);
@@ -1023,7 +1021,14 @@ void virNetServerClientClose(virNetServerClientPtr client)
         virObjectUnref(client->sock);
         client->sock = NULL;
     }
+}
 
+
+void
+virNetServerClientClose(virNetServerClientPtr client)
+{
+    virObjectLock(client);
+    virNetServerClientCloseLocked(client);
     virObjectUnlock(client);
 }
 
@@ -1051,13 +1056,11 @@ void virNetServerClientImmediateClose(virNetServerClientPtr client)
     virObjectUnlock(client);
 }
 
-bool virNetServerClientWantClose(virNetServerClientPtr client)
+
+/* @client needs to be locked by the caller */
+bool virNetServerClientWantCloseLocked(virNetServerClientPtr client)
 {
-    bool wantClose;
-    virObjectLock(client);
-    wantClose = client->wantClose;
-    virObjectUnlock(client);
-    return wantClose;
+    return client->wantClose;
 }
 
 
diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
index e45c78882ef7..60ad0f9ed326 100644
--- a/src/rpc/virnetserverclient.h
+++ b/src/rpc/virnetserverclient.h
@@ -123,11 +123,12 @@ void virNetServerClientSetDispatcher(virNetServerClientPtr client,
                                      virNetServerClientDispatchFunc func,
                                      void *opaque);
 void virNetServerClientClose(virNetServerClientPtr client);
+void virNetServerClientCloseLocked(virNetServerClientPtr client);
 bool virNetServerClientIsClosed(virNetServerClientPtr client);
 
 void virNetServerClientDelayedClose(virNetServerClientPtr client);
 void virNetServerClientImmediateClose(virNetServerClientPtr client);
-bool virNetServerClientWantClose(virNetServerClientPtr client);
+bool virNetServerClientWantCloseLocked(virNetServerClientPtr client);
 
 int virNetServerClientInit(virNetServerClientPtr client);
 
-- 
2.13.4

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list