Re: [libvirt] [PATCH libvirt v2 6/9] remote/rpc: Use virNetServerGetProgram() to determine the program

[Marc Hartmayer]

On Thu, Apr 26, 2018 at 05:08 PM +0200, John Ferlan  wrote:
> On 04/12/2018 08:41 AM, Marc Hartmayer wrote:
>> Use virNetServerGetProgram() to determine the virNetServerProgram
>> instead of using hard coded global variables. This allows us to remove
>> the global variables @remoteProgram and @qemuProgram as they're now no
>> longer necessary.
>>
>> Signed-off-by: Marc Hartmayer 
>> Reviewed-by: Boris Fiuczynski 
>> ---
>>
>> Note: I'm not 100% sure that there is no case where the lock for
>> @client is already held by the thread which is calling
>> virNetServerGetProgram and thus the lock order would be violated (the
>> lock order has to be @server -> @client in the violating case it would
>> be @client -> @server and therefore a deadlock might occur).
>>
>
> Well the way I read virNetServerProgramDispatchCall it would seem both
> @server and @client must be unlocked:
>
> /*
>  * When the RPC handler is called:
>  *
>  *  - Server object is unlocked
>  *  - Client object is unlocked
>  *
>  * Without locking, it is safe to use:
>  *
>  *   'args and 'ret'
>  */
> rv = (dispatcher->func)(server, client, msg, , arg, ret);

Thanks for digging into that.

>
>> ---
>>  src/libvirt_remote.syms |   1 +
>>  src/remote/remote_daemon.c  |   4 +-
>>  src/remote/remote_daemon.h  |   3 -
>>  src/remote/remote_daemon_dispatch.c | 116 
>> +++-
>>  src/rpc/gendispatch.pl  |   6 ++
>>  src/rpc/virnetserver.c  |  23 +++
>>  src/rpc/virnetserver.h  |   2 +
>>  7 files changed, 122 insertions(+), 33 deletions(-)
>>
>
> Reviewed-by: John Ferlan 

…and thanks for the review.

>
> John
>
--
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

Re: [libvirt] [PATCH libvirt v2 6/9] remote/rpc: Use virNetServerGetProgram() to determine the program

[John Ferlan]

On 04/12/2018 08:41 AM, Marc Hartmayer wrote:
> Use virNetServerGetProgram() to determine the virNetServerProgram
> instead of using hard coded global variables. This allows us to remove
> the global variables @remoteProgram and @qemuProgram as they're now no
> longer necessary.
> 
> Signed-off-by: Marc Hartmayer 
> Reviewed-by: Boris Fiuczynski 
> ---
> 
> Note: I'm not 100% sure that there is no case where the lock for
> @client is already held by the thread which is calling
> virNetServerGetProgram and thus the lock order would be violated (the
> lock order has to be @server -> @client in the violating case it would
> be @client -> @server and therefore a deadlock might occur).
> 

Well the way I read virNetServerProgramDispatchCall it would seem both
@server and @client must be unlocked:

/*
 * When the RPC handler is called:
 *
 *  - Server object is unlocked
 *  - Client object is unlocked
 *
 * Without locking, it is safe to use:
 *
 *   'args and 'ret'
 */
rv = (dispatcher->func)(server, client, msg, , arg, ret);

> ---
>  src/libvirt_remote.syms |   1 +
>  src/remote/remote_daemon.c  |   4 +-
>  src/remote/remote_daemon.h  |   3 -
>  src/remote/remote_daemon_dispatch.c | 116 
> +++-
>  src/rpc/gendispatch.pl  |   6 ++
>  src/rpc/virnetserver.c  |  23 +++
>  src/rpc/virnetserver.h  |   2 +
>  7 files changed, 122 insertions(+), 33 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

[libvirt] [PATCH libvirt v2 6/9] remote/rpc: Use virNetServerGetProgram() to determine the program

[Marc Hartmayer]

Use virNetServerGetProgram() to determine the virNetServerProgram
instead of using hard coded global variables. This allows us to remove
the global variables @remoteProgram and @qemuProgram as they're now no
longer necessary.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Boris Fiuczynski 
---

Note: I'm not 100% sure that there is no case where the lock for
@client is already held by the thread which is calling
virNetServerGetProgram and thus the lock order would be violated (the
lock order has to be @server -> @client in the violating case it would
be @client -> @server and therefore a deadlock might occur).

---
 src/libvirt_remote.syms |   1 +
 src/remote/remote_daemon.c  |   4 +-
 src/remote/remote_daemon.h  |   3 -
 src/remote/remote_daemon_dispatch.c | 116 +++-
 src/rpc/gendispatch.pl  |   6 ++
 src/rpc/virnetserver.c  |  23 +++
 src/rpc/virnetserver.h  |   2 +
 7 files changed, 122 insertions(+), 33 deletions(-)

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 97e22275b980..c31b16cd5909 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -120,6 +120,7 @@ virNetServerGetCurrentUnauthClients;
 virNetServerGetMaxClients;
 virNetServerGetMaxUnauthClients;
 virNetServerGetName;
+virNetServerGetProgram;
 virNetServerGetThreadPoolParameters;
 virNetServerHasClients;
 virNetServerNew;
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 31c6ce1b6179..f854a1a6981e 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -71,8 +71,6 @@ VIR_LOG_INIT("daemon.libvirtd");
 #if WITH_SASL
 virNetSASLContextPtr saslCtxt = NULL;
 #endif
-virNetServerProgramPtr remoteProgram = NULL;
-virNetServerProgramPtr qemuProgram = NULL;
 
 volatile bool driversInitialized = false;
 
@@ -1049,6 +1047,8 @@ int main(int argc, char **argv) {
 virNetServerPtr srv = NULL;
 virNetServerPtr srvAdm = NULL;
 virNetServerProgramPtr adminProgram = NULL;
+virNetServerProgramPtr qemuProgram = NULL;
+virNetServerProgramPtr remoteProgram = NULL;
 virNetServerProgramPtr lxcProgram = NULL;
 char *remote_config_file = NULL;
 int statuswrite = -1;
diff --git a/src/remote/remote_daemon.h b/src/remote/remote_daemon.h
index 2834da04a9ae..a2eda209683b 100644
--- a/src/remote/remote_daemon.h
+++ b/src/remote/remote_daemon.h
@@ -88,7 +88,4 @@ struct daemonClientPrivate {
 # if WITH_SASL
 extern virNetSASLContextPtr saslCtxt;
 # endif
-extern virNetServerProgramPtr remoteProgram;
-extern virNetServerProgramPtr qemuProgram;
-
 #endif /* __REMOTE_DAEMON_H__ */
diff --git a/src/remote/remote_daemon_dispatch.c 
b/src/remote/remote_daemon_dispatch.c
index cf2cd0add7d6..94b9cc3377d8 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -3830,15 +3830,16 @@ remoteDispatchNodeDeviceGetParent(virNetServerPtr 
server ATTRIBUTE_UNUSED,
 }
 
 static int
-remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server 
ATTRIBUTE_UNUSED,
+remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server,
virNetServerClientPtr client,
-   virNetMessagePtr msg 
ATTRIBUTE_UNUSED,
+   virNetMessagePtr msg,
virNetMessageErrorPtr rerr)
 {
 int rv = -1;
 daemonClientEventCallbackPtr callback = NULL;
 struct daemonClientPrivate *priv =
 virNetServerClientGetPrivateData(client);
+virNetServerProgramPtr program;
 
 virMutexLock(>lock);
 
@@ -3847,10 +3848,15 @@ 
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
 goto cleanup;
 }
 
+if (!(program = virNetServerGetProgram(server, msg))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program 
found"));
+goto cleanup;
+}
+
 if (VIR_ALLOC(callback) < 0)
 goto cleanup;
 callback->client = virObjectRef(client);
-callback->program = virObjectRef(remoteProgram);
+callback->program = virObjectRef(program);
 /* eventID, callbackID, and legacy are not used */
 callback->eventID = -1;
 callback->callbackID = -1;
@@ -3903,9 +3909,9 @@ 
remoteDispatchConnectUnregisterCloseCallback(virNetServerPtr server ATTRIBUTE_UN
 }
 
 static int
-remoteDispatchConnectDomainEventRegister(virNetServerPtr server 
ATTRIBUTE_UNUSED,
+remoteDispatchConnectDomainEventRegister(virNetServerPtr server,
  virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
  virNetMessageErrorPtr rerr 
ATTRIBUTE_UNUSED,