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

[libvirt] [PATCH 10/14] rpc: Introduce virNetServerSetClientAuthenticated

Marc Hartmayer posted 14 patches 7 years, 5 months ago
There is a newer version of this series
[libvirt] [PATCH 10/14] rpc: Introduce virNetServerSetClientAuthenticated
Posted by Marc Hartmayer 7 years, 5 months ago 
Introduce a function which marks the client as authenticated and also
it tracks on the server that the authentication for this client has
been completed. Afterwords it will check for the limits of the server.

After using this new function the function
virNetServerTrackCompletedAuth was superfluous and is therefore
removed. In addition, it is not very common that a
'{{function}}' (virNetServerTrackCompletedAuth) does more than just
the locking compared to
'{{function}}Locked' (virNetServerTrackCompletedAuthLocked).

virNetServerTrackPendingAuth was already superfluous and therefore
it's also removed.

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>
---
 daemon/remote.c              |  9 +++------
 src/libvirt_remote.syms      |  5 ++---
 src/rpc/virnetserver.c       | 40 ++++++++++++++++++++++------------------
 src/rpc/virnetserver.h       |  3 +--
 src/rpc/virnetserverclient.c |  5 ++---
 src/rpc/virnetserverclient.h |  2 +-
 6 files changed, 31 insertions(+), 33 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index 6f78f17178f7..a9dd228b8273 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -3263,8 +3263,7 @@ remoteDispatchAuthList(virNetServerPtr server,
                             (long long) callerPid, (int) callerUid) < 0)
                 goto cleanup;
             VIR_INFO("Bypass polkit auth for privileged client %s", ident);
-            virNetServerClientSetAuth(client, VIR_NET_SERVER_SERVICE_AUTH_NONE);
-            virNetServerTrackCompletedAuth(server);
+            virNetServerSetClientAuthenticated(server, client);
             auth = VIR_NET_SERVER_SERVICE_AUTH_NONE;
             VIR_FREE(ident);
         }
@@ -3407,8 +3406,7 @@ remoteSASLFinish(virNetServerPtr server,
     if (!(clnt_identity = virNetServerClientGetIdentity(client)))
         goto error;
 
-    virNetServerClientSetAuth(client, VIR_NET_SERVER_SERVICE_AUTH_NONE);
-    virNetServerTrackCompletedAuth(server);
+    virNetServerSetClientAuthenticated(server, client);
     virNetServerClientSetSASLSession(client, priv->sasl);
     virIdentitySetSASLUserName(clnt_identity, identity);
 
@@ -3731,8 +3729,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server,
              action, (long long) callerPid, callerUid);
     ret->complete = 1;
 
-    virNetServerClientSetAuth(client, VIR_NET_SERVER_SERVICE_AUTH_NONE);
-    virNetServerTrackCompletedAuth(server);
+    virNetServerSetClientAuthenticated(server, client);
     virMutexUnlock(&priv->lock);
 
     return 0;
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index efdf912ec563..7fcae970484d 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -116,9 +116,8 @@ virNetServerNewPostExecRestart;
 virNetServerNextClientID;
 virNetServerPreExecRestart;
 virNetServerProcessClients;
+virNetServerSetClientAuthenticated;
 virNetServerStart;
-virNetServerTrackCompletedAuth;
-virNetServerTrackPendingAuth;
 virNetServerUpdateServices;
 
 
@@ -152,7 +151,7 @@ virNetServerClientRemoteAddrStringSASL;
 virNetServerClientRemoteAddrStringURI;
 virNetServerClientRemoveFilter;
 virNetServerClientSendMessage;
-virNetServerClientSetAuth;
+virNetServerClientSetAuthLocked;
 virNetServerClientSetCloseHook;
 virNetServerClientSetDispatcher;
 virNetServerClientStartKeepAlive;
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index d03bd3e91905..72105cd9318f 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -737,6 +737,28 @@ int virNetServerSetTLSContext(virNetServerPtr srv,
 #endif
 
 
+/**
+ * virNetServerSetClientAuthenticated:
+ * @srv: server must be unlocked
+ * @client: client must be unlocked
+ *
+ * Mark @client as authenticated and tracks on @srv that the
+ * authentication of this @client has been completed. Also it checks
+ * the limits of @srv.
+ */
+void
+virNetServerSetClientAuthenticated(virNetServerPtr srv, virNetServerClientPtr client)
+{
+    virObjectLock(srv);
+    virObjectLock(client);
+    virNetServerClientSetAuthLocked(client, VIR_NET_SERVER_SERVICE_AUTH_NONE);
+    virNetServerTrackCompletedAuthLocked(srv);
+    virNetServerCheckLimits(srv);
+    virObjectUnlock(client);
+    virObjectUnlock(srv);
+}
+
+
 static void
 virNetServerUpdateServicesLocked(virNetServerPtr srv,
                                  bool enabled)
@@ -813,24 +835,6 @@ virNetServerTrackCompletedAuthLocked(virNetServerPtr srv)
     return --srv->nclients_unauth;
 }
 
-size_t virNetServerTrackPendingAuth(virNetServerPtr srv)
-{
-    size_t ret;
-    virObjectLock(srv);
-    ret = virNetServerTrackPendingAuthLocked(srv);
-    virObjectUnlock(srv);
-    return ret;
-}
-
-size_t virNetServerTrackCompletedAuth(virNetServerPtr srv)
-{
-    size_t ret;
-    virObjectLock(srv);
-    ret = virNetServerTrackCompletedAuthLocked(srv);
-    virNetServerCheckLimits(srv);
-    virObjectUnlock(srv);
-    return ret;
-}
 
 bool
 virNetServerHasClients(virNetServerPtr srv)
diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
index 6a79d15370e5..7728a67f5fcb 100644
--- a/src/rpc/virnetserver.h
+++ b/src/rpc/virnetserver.h
@@ -73,13 +73,12 @@ int virNetServerSetTLSContext(virNetServerPtr srv,
                               virNetTLSContextPtr tls);
 # endif
 
-size_t virNetServerTrackPendingAuth(virNetServerPtr srv);
-size_t virNetServerTrackCompletedAuth(virNetServerPtr srv);
 
 int virNetServerAddClient(virNetServerPtr srv,
                           virNetServerClientPtr client);
 bool virNetServerHasClients(virNetServerPtr srv);
 void virNetServerProcessClients(virNetServerPtr srv);
+void virNetServerSetClientAuthenticated(virNetServerPtr srv, virNetServerClientPtr client);
 
 void virNetServerUpdateServices(virNetServerPtr srv, bool enabled);
 
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 3101b555a90d..6adc3ec3ec01 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -639,11 +639,10 @@ int virNetServerClientGetAuth(virNetServerClientPtr client)
     return auth;
 }
 
-void virNetServerClientSetAuth(virNetServerClientPtr client, int auth)
+/* @client needs to be locked by the caller */
+void virNetServerClientSetAuthLocked(virNetServerClientPtr client, int auth)
 {
-    virObjectLock(client);
     client->auth = auth;
-    virObjectUnlock(client);
 }
 
 bool virNetServerClientGetReadonly(virNetServerClientPtr client)
diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
index 9ec18588a858..19f20b808284 100644
--- a/src/rpc/virnetserverclient.h
+++ b/src/rpc/virnetserverclient.h
@@ -79,7 +79,7 @@ void virNetServerClientRemoveFilter(virNetServerClientPtr client,
                                     int filterID);
 
 int virNetServerClientGetAuth(virNetServerClientPtr client);
-void virNetServerClientSetAuth(virNetServerClientPtr client, int auth);
+void virNetServerClientSetAuthLocked(virNetServerClientPtr client, int auth);
 bool virNetServerClientGetReadonly(virNetServerClientPtr client);
 unsigned long long virNetServerClientGetID(virNetServerClientPtr client);
 long long virNetServerClientGetTimestamp(virNetServerClientPtr client);
-- 
2.13.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/14] rpc: Introduce virNetServerSetClientAuthenticated
Posted by John Ferlan 7 years, 5 months ago 

On 12/12/2017 06:36 AM, Marc Hartmayer wrote:
> Introduce a function which marks the client as authenticated and also
> it tracks on the server that the authentication for this client has
> been completed. Afterwords it will check for the limits of the server.
> 
> After using this new function the function
> virNetServerTrackCompletedAuth was superfluous and is therefore
> removed. In addition, it is not very common that a
> '{{function}}' (virNetServerTrackCompletedAuth) does more than just
> the locking compared to
> '{{function}}Locked' (virNetServerTrackCompletedAuthLocked).
> 
> virNetServerTrackPendingAuth was already superfluous and therefore
> it's also removed.
> 

So essentially you're combining virNetServerClientSetAuth and
virNetServerTrackCompletedAuth into one new function and needed to
rename the virNetServerClientSetAuth to the Locked variety.

> 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>
> ---
>  daemon/remote.c              |  9 +++------
>  src/libvirt_remote.syms      |  5 ++---
>  src/rpc/virnetserver.c       | 40 ++++++++++++++++++++++------------------
>  src/rpc/virnetserver.h       |  3 +--
>  src/rpc/virnetserverclient.c |  5 ++---
>  src/rpc/virnetserverclient.h |  2 +-
>  6 files changed, 31 insertions(+), 33 deletions(-)
> 

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

John

one nit below...

[...]

> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index d03bd3e91905..72105cd9318f 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -737,6 +737,28 @@ int virNetServerSetTLSContext(virNetServerPtr srv,
>  #endif
>  
>  
> +/**
> + * virNetServerSetClientAuthenticated:
> + * @srv: server must be unlocked
> + * @client: client must be unlocked
> + *
> + * Mark @client as authenticated and tracks on @srv that the
> + * authentication of this @client has been completed. Also it checks
> + * the limits of @srv.
> + */
> +void
> +virNetServerSetClientAuthenticated(virNetServerPtr srv, virNetServerClientPtr client)

One line for each argument...

> +{
> +    virObjectLock(srv);
> +    virObjectLock(client);
> +    virNetServerClientSetAuthLocked(client, VIR_NET_SERVER_SERVICE_AUTH_NONE);
> +    virNetServerTrackCompletedAuthLocked(srv);
> +    virNetServerCheckLimits(srv);
> +    virObjectUnlock(client);
> +    virObjectUnlock(srv);
> +}
> +
> +

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/14] rpc: Introduce virNetServerSetClientAuthenticated
Posted by Marc Hartmayer 7 years, 4 months ago 
On Fri, Dec 15, 2017 at 03:26 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
> On 12/12/2017 06:36 AM, Marc Hartmayer wrote:
>> Introduce a function which marks the client as authenticated and also
>> it tracks on the server that the authentication for this client has
>> been completed. Afterwords it will check for the limits of the server.
>>
>> After using this new function the function
>> virNetServerTrackCompletedAuth was superfluous and is therefore
>> removed. In addition, it is not very common that a
>> '{{function}}' (virNetServerTrackCompletedAuth) does more than just
>> the locking compared to
>> '{{function}}Locked' (virNetServerTrackCompletedAuthLocked).
>>
>> virNetServerTrackPendingAuth was already superfluous and therefore
>> it's also removed.
>>
>
> So essentially you're combining virNetServerClientSetAuth and
> virNetServerTrackCompletedAuth into one new function and needed to
> rename the virNetServerClientSetAuth to the Locked variety.

Yes. I’ll rewrite the commit message.

>
>> 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>
>> ---
>>  daemon/remote.c              |  9 +++------
>>  src/libvirt_remote.syms      |  5 ++---
>>  src/rpc/virnetserver.c       | 40 ++++++++++++++++++++++------------------
>>  src/rpc/virnetserver.h       |  3 +--
>>  src/rpc/virnetserverclient.c |  5 ++---
>>  src/rpc/virnetserverclient.h |  2 +-
>>  6 files changed, 31 insertions(+), 33 deletions(-)
>>
>
> Reviewed-by: John Ferlan <jferlan@redhat.com>

Thanks.

>
> John
>
> one nit below...
>
> [...]
>
>> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
>> index d03bd3e91905..72105cd9318f 100644
>> --- a/src/rpc/virnetserver.c
>> +++ b/src/rpc/virnetserver.c
>> @@ -737,6 +737,28 @@ int virNetServerSetTLSContext(virNetServerPtr srv,
>>  #endif
>>
>>
>> +/**
>> + * virNetServerSetClientAuthenticated:
>> + * @srv: server must be unlocked
>> + * @client: client must be unlocked
>> + *
>> + * Mark @client as authenticated and tracks on @srv that the
>> + * authentication of this @client has been completed. Also it checks
>> + * the limits of @srv.
>> + */
>> +void
>> +virNetServerSetClientAuthenticated(virNetServerPtr srv, virNetServerClientPtr client)
>
> One line for each argument...
>
>> +{
>> +    virObjectLock(srv);
>> +    virObjectLock(client);
>> +    virNetServerClientSetAuthLocked(client, VIR_NET_SERVER_SERVICE_AUTH_NONE);
>> +    virNetServerTrackCompletedAuthLocked(srv);
>> +    virNetServerCheckLimits(srv);
>> +    virObjectUnlock(client);
>> +    virObjectUnlock(srv);
>> +}
>> +
>> +
>
> [...]
>
--
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.