1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 00/14] Fix race on srv->nclients_unauth and some other changes
  4. View patch

[libvirt] [PATCH 09/14] rpc: Merge critical sections and preparations for upcoming patches

Marc Hartmayer posted 14 patches 7 years, 5 months ago
There is a newer version of this series
[libvirt] [PATCH 09/14] rpc: Merge critical sections and preparations for upcoming patches
Posted by Marc Hartmayer 7 years, 5 months ago 
Merge critical sections for virNetServerProcessClients and take the
lock of @client before we're calling virNetServerClientNeedAuth. This
is done in preparation for upcoming patches and in addition it is more
efficient not to release a lock which is reacquired immediately
afterwards.

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 | 22 ++++++++++++++--------
 src/rpc/virnetserverclient.h |  3 ++-
 4 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 3ce5694b781d..efdf912ec563 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -139,11 +139,12 @@ virNetServerClientGetUNIXIdentity;
 virNetServerClientImmediateClose;
 virNetServerClientInit;
 virNetServerClientInitKeepAlive;
-virNetServerClientIsClosed;
+virNetServerClientIsClosedLocked;
 virNetServerClientIsLocal;
 virNetServerClientIsSecure;
 virNetServerClientLocalAddrStringSASL;
 virNetServerClientNeedAuth;
+virNetServerClientNeedAuthLocked;
 virNetServerClientNew;
 virNetServerClientNewPostExecRestart;
 virNetServerClientPreExecRestart;
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index dde9b73fc250..d03bd3e91905 100644
--- 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);
 
@@ -860,13 +862,13 @@ virNetServerProcessClients(virNetServerPtr srv)
         virObjectLock(client);
         if (virNetServerClientWantCloseLocked(client))
             virNetServerClientCloseLocked(client);
-        virObjectUnlock(client);
 
-        if (virNetServerClientIsClosed(client)) {
+        if (virNetServerClientIsClosedLocked(client)) {
             VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients);
 
-            if (virNetServerClientNeedAuth(client))
+            if (virNetServerClientNeedAuthLocked(client))
                 virNetServerTrackCompletedAuthLocked(srv);
+            virObjectUnlock(client);
 
             virNetServerCheckLimits(srv);
 
@@ -875,6 +877,8 @@ virNetServerProcessClients(virNetServerPtr srv)
             virObjectLock(srv);
 
             goto reprocess;
+        } else {
+            virObjectUnlock(client);
         }
     }
 
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 616b6fe115e5..3101b555a90d 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -1050,13 +1050,10 @@ virNetServerClientClose(virNetServerClientPtr client)
 }
 
 
-bool virNetServerClientIsClosed(virNetServerClientPtr client)
+/* @client needs to be locked by the caller */
+bool virNetServerClientIsClosedLocked(virNetServerClientPtr client)
 {
-    bool closed;
-    virObjectLock(client);
-    closed = client->sock == NULL ? true : false;
-    virObjectUnlock(client);
-    return closed;
+    return client->sock == NULL ? true : false;
 }
 
 void virNetServerClientDelayedClose(virNetServerClientPtr client)
@@ -1530,11 +1527,20 @@ int virNetServerClientSendMessage(virNetServerClientPtr client,
 }
 
 
-bool virNetServerClientNeedAuth(virNetServerClientPtr client)
+/* The caller must hold the lock for @client */
+bool
+virNetServerClientNeedAuthLocked(virNetServerClientPtr client)
+{
+    return !virNetServerClientAuthMethodImpliesAuthenticated(client->auth);
+}
+
+
+bool
+virNetServerClientNeedAuth(virNetServerClientPtr client)
 {
     bool need;
     virObjectLock(client);
-    need = !virNetServerClientAuthMethodImpliesAuthenticated(client->auth);
+    need = virNetServerClientNeedAuthLocked(client);
     virObjectUnlock(client);
     return need;
 }
diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
index 60ad0f9ed326..9ec18588a858 100644
--- a/src/rpc/virnetserverclient.h
+++ b/src/rpc/virnetserverclient.h
@@ -124,7 +124,7 @@ void virNetServerClientSetDispatcher(virNetServerClientPtr client,
                                      void *opaque);
 void virNetServerClientClose(virNetServerClientPtr client);
 void virNetServerClientCloseLocked(virNetServerClientPtr client);
-bool virNetServerClientIsClosed(virNetServerClientPtr client);
+bool virNetServerClientIsClosedLocked(virNetServerClientPtr client);
 
 void virNetServerClientDelayedClose(virNetServerClientPtr client);
 void virNetServerClientImmediateClose(virNetServerClientPtr client);
@@ -147,6 +147,7 @@ int virNetServerClientSendMessage(virNetServerClientPtr client,
                                   virNetMessagePtr msg);
 
 bool virNetServerClientNeedAuth(virNetServerClientPtr client);
+bool virNetServerClientNeedAuthLocked(virNetServerClientPtr client);
 
 int virNetServerClientGetTransport(virNetServerClientPtr client);
 int virNetServerClientGetInfo(virNetServerClientPtr client,
-- 
2.13.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/14] rpc: Merge critical sections and preparations for upcoming patches
Posted by John Ferlan 7 years, 5 months ago 

On 12/12/2017 06:36 AM, Marc Hartmayer wrote:
> Merge critical sections for virNetServerProcessClients and take the
> lock of @client before we're calling virNetServerClientNeedAuth. This
> is done in preparation for upcoming patches and in addition it is more
> efficient not to release a lock which is reacquired immediately
> afterwards.
> 
> 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 | 22 ++++++++++++++--------
>  src/rpc/virnetserverclient.h |  3 ++-
>  4 files changed, 26 insertions(+), 14 deletions(-)
> 

I think if you split out the virNetServerClientIsClosedLocked into its
own patch before/after the virNetServerClientNeedAuthLocked, then
perhaps patch 7 and 9 are combined.

It's just a patch ordering and trying not to do too much in one patch.

> diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
> index 3ce5694b781d..efdf912ec563 100644
> --- a/src/libvirt_remote.syms
> +++ b/src/libvirt_remote.syms
> @@ -139,11 +139,12 @@ virNetServerClientGetUNIXIdentity;
>  virNetServerClientImmediateClose;
>  virNetServerClientInit;
>  virNetServerClientInitKeepAlive;
> -virNetServerClientIsClosed;
> +virNetServerClientIsClosedLocked;
>  virNetServerClientIsLocal;
>  virNetServerClientIsSecure;
>  virNetServerClientLocalAddrStringSASL;
>  virNetServerClientNeedAuth;
> +virNetServerClientNeedAuthLocked;
>  virNetServerClientNew;
>  virNetServerClientNewPostExecRestart;
>  virNetServerClientPreExecRestart;
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index dde9b73fc250..d03bd3e91905 100644
> --- 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);
>  
> @@ -860,13 +862,13 @@ virNetServerProcessClients(virNetServerPtr srv)
>          virObjectLock(client);
>          if (virNetServerClientWantCloseLocked(client))
>              virNetServerClientCloseLocked(client);
> -        virObjectUnlock(client);
>  
> -        if (virNetServerClientIsClosed(client)) {
> +        if (virNetServerClientIsClosedLocked(client)) {
>              VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients);
>  
> -            if (virNetServerClientNeedAuth(client))
> +            if (virNetServerClientNeedAuthLocked(client))
>                  virNetServerTrackCompletedAuthLocked(srv);
> +            virObjectUnlock(client);
>  
>              virNetServerCheckLimits(srv);
>  
> @@ -875,6 +877,8 @@ virNetServerProcessClients(virNetServerPtr srv)
>              virObjectLock(srv);
>  
>              goto reprocess;
> +        } else {
> +            virObjectUnlock(client);
>          }
>      }
>  
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> index 616b6fe115e5..3101b555a90d 100644
> --- a/src/rpc/virnetserverclient.c
> +++ b/src/rpc/virnetserverclient.c
> @@ -1050,13 +1050,10 @@ virNetServerClientClose(virNetServerClientPtr client)
>  }
>  
>  
> -bool virNetServerClientIsClosed(virNetServerClientPtr client)
> +/* @client needs to be locked by the caller */
> +bool virNetServerClientIsClosedLocked(virNetServerClientPtr client)

May as well go with the multi line here too

bool
virNetServer...

John

>  {
> -    bool closed;
> -    virObjectLock(client);
> -    closed = client->sock == NULL ? true : false;
> -    virObjectUnlock(client);
> -    return closed;
> +    return client->sock == NULL ? true : false;
>  }
>  
>  void virNetServerClientDelayedClose(virNetServerClientPtr client)
> @@ -1530,11 +1527,20 @@ int virNetServerClientSendMessage(virNetServerClientPtr client,
>  }
>  
>  
> -bool virNetServerClientNeedAuth(virNetServerClientPtr client)
> +/* The caller must hold the lock for @client */
> +bool
> +virNetServerClientNeedAuthLocked(virNetServerClientPtr client)
> +{
> +    return !virNetServerClientAuthMethodImpliesAuthenticated(client->auth);
> +}
> +
> +
> +bool
> +virNetServerClientNeedAuth(virNetServerClientPtr client)
>  {
>      bool need;
>      virObjectLock(client);
> -    need = !virNetServerClientAuthMethodImpliesAuthenticated(client->auth);
> +    need = virNetServerClientNeedAuthLocked(client);
>      virObjectUnlock(client);
>      return need;
>  }
> diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
> index 60ad0f9ed326..9ec18588a858 100644
> --- a/src/rpc/virnetserverclient.h
> +++ b/src/rpc/virnetserverclient.h
> @@ -124,7 +124,7 @@ void virNetServerClientSetDispatcher(virNetServerClientPtr client,
>                                       void *opaque);
>  void virNetServerClientClose(virNetServerClientPtr client);
>  void virNetServerClientCloseLocked(virNetServerClientPtr client);
> -bool virNetServerClientIsClosed(virNetServerClientPtr client);
> +bool virNetServerClientIsClosedLocked(virNetServerClientPtr client);
>  
>  void virNetServerClientDelayedClose(virNetServerClientPtr client);
>  void virNetServerClientImmediateClose(virNetServerClientPtr client);
> @@ -147,6 +147,7 @@ int virNetServerClientSendMessage(virNetServerClientPtr client,
>                                    virNetMessagePtr msg);
>  
>  bool virNetServerClientNeedAuth(virNetServerClientPtr client);
> +bool virNetServerClientNeedAuthLocked(virNetServerClientPtr client);
>  
>  int virNetServerClientGetTransport(virNetServerClientPtr client);
>  int virNetServerClientGetInfo(virNetServerClientPtr client,
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/14] rpc: Merge critical sections and preparations for upcoming patches
Posted by Marc Hartmayer 7 years, 4 months ago 
On Fri, Dec 15, 2017 at 02:53 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
> On 12/12/2017 06:36 AM, Marc Hartmayer wrote:
>> Merge critical sections for virNetServerProcessClients and take the
>> lock of @client before we're calling virNetServerClientNeedAuth. This
>> is done in preparation for upcoming patches and in addition it is more
>> efficient not to release a lock which is reacquired immediately
>> afterwards.
>>
>> 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 | 22 ++++++++++++++--------
>>  src/rpc/virnetserverclient.h |  3 ++-
>>  4 files changed, 26 insertions(+), 14 deletions(-)
>>
>
> I think if you split out the virNetServerClientIsClosedLocked into its
> own patch before/after the virNetServerClientNeedAuthLocked, then
> perhaps patch 7 and 9 are combined.

Done.

>
> It's just a patch ordering and trying not to do too much in one patch.
>
>> diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
>> index 3ce5694b781d..efdf912ec563 100644
>> --- a/src/libvirt_remote.syms
>> +++ b/src/libvirt_remote.syms

[…snip]

--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


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

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.