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>

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

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);
         }
     }
 

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) {
+    if (!client->sock)
-        virObjectUnlock(client);
         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;
+    return client->wantClose;
-    virObjectLock(client);
-    wantClose = client->wantClose;
-    virObjectUnlock(client);
-    return wantClose;
 }
 
 

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);