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@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com>
---
 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 12/12/2017 06:36 AM, Marc Hartmayer wrote:
> 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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com>
> ---
>  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(-)
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

(minor nit below)

[...]

> 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

[...]

>  
> -bool virNetServerClientWantClose(virNetServerClientPtr client)
> +
> +/* @client needs to be locked by the caller */
> +bool virNetServerClientWantCloseLocked(virNetServerClientPtr client)

Seeing as you added the "newer" extra line between functions you could
also have used the "newer" multi-line function decl:

bool
virNetServerClient...



>  {
> -    bool wantClose;
> -    virObjectLock(client);
> -    wantClose = client->wantClose;
> -    virObjectUnlock(client);
> -    return wantClose;
> +    return client->wantClose;
>  }
>  
>  

[...]

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