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

[John Ferlan]

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 at linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
> Reviewed-by: Stefan Zimmermann <stzi at 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,
>