rpc: Correct locking and simplify the function

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. The same applies to the tracking of
authentications.

Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index cecd71c49e7f7bc2ee5d7d546c8c4ef28e4f942e..4e684ef69514172af348037a4b70f4a4f0217f26 100644 (file)
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -125,6 +125,7 @@ virNetServerUpdateServices;
 # rpc/virnetserverclient.h
 virNetServerClientAddFilter;
 virNetServerClientClose;
+virNetServerClientCloseLocked;
 virNetServerClientDelayedClose;
 virNetServerClientGetAuth;
 virNetServerClientGetFD;
@@ -138,7 +139,7 @@ virNetServerClientGetUNIXIdentity;
 virNetServerClientImmediateClose;
 virNetServerClientInit;
 virNetServerClientInitKeepAlive;
-virNetServerClientIsClosed;
+virNetServerClientIsClosedLocked;
 virNetServerClientIsLocal;
 virNetServerClientIsSecure;
 virNetServerClientLocalAddrStringSASL;
@@ -156,7 +157,7 @@ virNetServerClientSetCloseHook;
 virNetServerClientSetDispatcher;
 virNetServerClientSetReadonly;
 virNetServerClientStartKeepAlive;
-virNetServerClientWantClose;
+virNetServerClientWantCloseLocked;
 
 
 # rpc/virnetservermdns.h

diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 43f889e2a037a70048d48a56df31fd462a61099d..57cbfb59ab53dba140c3383b08a4bfe0dc094c17 100644 (file)
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -285,8 +285,10 @@ int virNetServerAddClient(virNetServerPtr srv,
         goto error;
     srv->clients[srv->nclients-1] = virObjectRef(client);
 
-    if (virNetServerClientNeedAuth(client))
+    virObjectLock(client);
+    if (virNetServerClientNeedAuthLocked(client))
         virNetServerTrackPendingAuthLocked(srv);
+    virObjectUnlock(client);
 
     virNetServerCheckLimits(srv);
 
@@ -847,6 +849,7 @@ void
 virNetServerProcessClients(virNetServerPtr srv)
 {
     size_t i;
+    virNetServerClientPtr client;
 
     virObjectLock(srv);
 
@@ -855,15 +858,18 @@ 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);
+
+        if (virNetServerClientIsClosedLocked(client)) {
             VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients);
 
-            if (virNetServerClientNeedAuth(client))
+            if (virNetServerClientNeedAuthLocked(client))
                 virNetServerTrackCompletedAuthLocked(srv);
+            virObjectUnlock(client);
 
             virNetServerCheckLimits(srv);
 
@@ -872,6 +878,8 @@ virNetServerProcessClients(virNetServerPtr srv)
             virObjectLock(srv);
 
             goto reprocess;
+        } else {
+            virObjectUnlock(client);
         }
     }
 

diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index afe4fb47a256807da2e5e7de1df37c66e934c17a..dee94450dfa31086b9490789b7d7cba4a2f9e8cb 100644 (file)
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -987,17 +987,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);
@@ -1048,20 +1046,25 @@ void virNetServerClientClose(virNetServerClientPtr client)
         virObjectUnref(client->sock);
         client->sock = NULL;
     }
-
-    virObjectUnlock(client);
 }
 
 
-bool virNetServerClientIsClosed(virNetServerClientPtr client)
+void
+virNetServerClientClose(virNetServerClientPtr client)
 {
-    bool closed;
     virObjectLock(client);
-    closed = client->sock == NULL ? true : false;
+    virNetServerClientCloseLocked(client);
     virObjectUnlock(client);
-    return closed;
 }
 
+
+bool
+virNetServerClientIsClosedLocked(virNetServerClientPtr client)
+{
+    return client->sock == NULL ? true : false;
+}
+
+
 void virNetServerClientDelayedClose(virNetServerClientPtr client)
 {
     virObjectLock(client);
@@ -1076,13 +1079,11 @@ void virNetServerClientImmediateClose(virNetServerClientPtr client)
     virObjectUnlock(client);
 }
 
-bool virNetServerClientWantClose(virNetServerClientPtr client)
+
+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 1182d53c705978f87671f66222edba4fcb97b223..b7752a61fa8e498050f1432311e6002d4c56a1fa 100644 (file)
--- a/src/rpc/virnetserverclient.h
+++ b/src/rpc/virnetserverclient.h
@@ -124,11 +124,12 @@ void virNetServerClientSetDispatcher(virNetServerClientPtr client,
                                      virNetServerClientDispatchFunc func,
                                      void *opaque);
 void virNetServerClientClose(virNetServerClientPtr client);
-bool virNetServerClientIsClosed(virNetServerClientPtr client);
+void virNetServerClientCloseLocked(virNetServerClientPtr client);
+bool virNetServerClientIsClosedLocked(virNetServerClientPtr client);
 
 void virNetServerClientDelayedClose(virNetServerClientPtr client);
 void virNetServerClientImmediateClose(virNetServerClientPtr client);
-bool virNetServerClientWantClose(virNetServerClientPtr client);
+bool virNetServerClientWantCloseLocked(virNetServerClientPtr client);
 
 int virNetServerClientInit(virNetServerClientPtr client);