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