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

15 Mar 2018

On 03/08/2018 07:20 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 <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
---
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).
My quick look didn't see any, but this usually becomes a danpb type
question - that is "historically" has any entrance into
remote_daemon_dispatch.c API's already taken the client lock.
...
---
 src/libvirt_remote.syms             |  1 +
 src/remote/remote_daemon.c          |  4 +--
 src/remote/remote_daemon.h          |  3 ---
 src/remote/remote_daemon_dispatch.c | 52 ++++++++++++++++++-------------------
 src/rpc/gendispatch.pl              |  2 ++
 src/rpc/virnetserver.c              | 23 ++++++++++++++++
 src/rpc/virnetserver.h              |  2 ++
 7 files changed, 56 insertions(+), 31 deletions(-)
As noted in 19/20 - I wasn't able to git am -3 apply this, so I'll need
a v2...  From visual inspection things look good - I do have one
observation below...
...

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 078430f91c97..64f67ef2fefb 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;
@@ -1061,6 +1059,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 4467f71da910..93f2e1f531b0 100644
--- a/src/remote/remote_daemon.h
+++ b/src/remote/remote_daemon.h
@@ -82,7 +82,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 95940ffdeefe..3dce6195230e 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -3808,9 +3808,9 @@ 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;
@@ -3828,7 +3828,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(remoteProgram);
+    callback->program = virObjectRef(virNetServerGetProgram(server, msg));
So while it "shouldn't happen", virNetServerGetProgramLocked could
return NULL causing chaos...

John
...
/* eventID, callbackID, and legacy are not used */
     callback->eventID = -1;
     callback->callbackID = -1;
@@ -3912,7 +3912,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server ATTRIBUTE_UNUSED
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(remoteProgram);
+    callback->program = virObjectRef(virNetServerGetProgram(server, msg));
     callback->eventID = VIR_DOMAIN_EVENT_ID_LIFECYCLE;
     callback->callbackID = -1;
     callback->legacy = true;
@@ -4110,9 +4110,9 @@ remoteDispatchDomainGetState(virNetServerPtr server ATTRIBUTE_UNUSED,
  * VIR_DRV_SUPPORTS_FEATURE(VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK),
  * and must not mix the two styles.  */
 static int
-remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server,
                                             virNetServerClientPtr client,
-                                            virNetMessagePtr msg ATTRIBUTE_UNUSED,
+                                            virNetMessagePtr msg,
                                             virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
                                             remote_connect_domain_event_register_any_args *args)
 {
@@ -4149,7 +4149,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(remoteProgram);
+    callback->program = virObjectRef(virNetServerGetProgram(server, msg));
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     callback->legacy = true;
@@ -4185,9 +4185,9 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU
static int
-remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server,
                                                     virNetServerClientPtr client,
-                                                    virNetMessagePtr msg ATTRIBUTE_UNUSED,
+                                                    virNetMessagePtr msg,
                                                     virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
                                                     remote_connect_domain_event_callback_register_any_args *args,
                                                     remote_connect_domain_event_callback_register_any_ret *ret)
@@ -4226,7 +4226,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server ATTRI
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(remoteProgram);
+    callback->program = virObjectRef(virNetServerGetProgram(server, msg));
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     ref = callback;
@@ -5352,7 +5352,7 @@ remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr server ATTRIBUTE
         goto cleanup;
if (!(st = virStreamNew(priv->conn, VIR_STREAM_NONBLOCK)) ||
-        !(stream = daemonCreateClientStream(client, st, remoteProgram,
+        !(stream = daemonCreateClientStream(client, st, virNetServerGetProgram(server, msg),
                                             &msg->header, false)))
         goto cleanup;
@@ -5709,9 +5709,9 @@ static int remoteDispatchDomainCreateWithFiles(virNetServerPtr server ATTRIBUTE_
static int
-remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server,
                                              virNetServerClientPtr client,
-                                             virNetMessagePtr msg ATTRIBUTE_UNUSED,
+                                             virNetMessagePtr msg,
                                              virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
                                              remote_connect_network_event_register_any_args *args,
                                              remote_connect_network_event_register_any_ret *ret)
@@ -5750,7 +5750,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server ATTRIBUTE_UN
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(remoteProgram);
+    callback->program = virObjectRef(virNetServerGetProgram(server, msg));
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     ref = callback;
@@ -5832,9 +5832,9 @@ remoteDispatchConnectNetworkEventDeregisterAny(virNetServerPtr server ATTRIBUTE_
 }
static int
-remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server,
                                                  virNetServerClientPtr client,
-                                                 virNetMessagePtr msg ATTRIBUTE_UNUSED,
+                                                 virNetMessagePtr msg,
                                                  virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
                                                  remote_connect_storage_pool_event_register_any_args *args,
                                                  remote_connect_storage_pool_event_register_any_ret *ret)
@@ -5873,7 +5873,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server ATTRIBUT
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(remoteProgram);
+    callback->program = virObjectRef(virNetServerGetProgram(server, msg));
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     ref = callback;
@@ -5954,9 +5954,9 @@ remoteDispatchConnectStoragePoolEventDeregisterAny(virNetServerPtr server ATTRIB
 }
static int
-remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server,
                                                 virNetServerClientPtr client,
-                                                virNetMessagePtr msg ATTRIBUTE_UNUSED,
+                                                virNetMessagePtr msg,
                                                 virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
                                                 remote_connect_node_device_event_register_any_args *args,
                                                 remote_connect_node_device_event_register_any_ret *ret)
@@ -5995,7 +5995,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server ATTRIBUTE
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(remoteProgram);
+    callback->program = virObjectRef(virNetServerGetProgram(server, msg));
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     ref = callback;
@@ -6076,9 +6076,9 @@ remoteDispatchConnectNodeDeviceEventDeregisterAny(virNetServerPtr server ATTRIBU
 }
static int
-remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server,
                                             virNetServerClientPtr client,
-                                            virNetMessagePtr msg ATTRIBUTE_UNUSED,
+                                            virNetMessagePtr msg,
                                             virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
                                             remote_connect_secret_event_register_any_args *args,
                                             remote_connect_secret_event_register_any_ret *ret)
@@ -6117,7 +6117,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(remoteProgram);
+    callback->program = virObjectRef(virNetServerGetProgram(server, msg));
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     ref = callback;
@@ -6198,9 +6198,9 @@ remoteDispatchConnectSecretEventDeregisterAny(virNetServerPtr server ATTRIBUTE_U
 }
static int
-qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server ATTRIBUTE_UNUSED,
+qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server,
                                               virNetServerClientPtr client,
-                                              virNetMessagePtr msg ATTRIBUTE_UNUSED,
+                                              virNetMessagePtr msg,
                                               virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
                                               qemu_connect_domain_monitor_event_register_args *args,
                                               qemu_connect_domain_monitor_event_register_ret *ret)
@@ -6234,7 +6234,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server ATTRIBUTE_U
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(qemuProgram);
+    callback->program = virObjectRef(virNetServerGetProgram(server, msg));
     callback->eventID = -1;
     callback->callbackID = -1;
     ref = callback;
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index fb15cc4849bc..725319bf8da4 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -990,6 +990,7 @@ elsif ($mode eq "server") {
         if ($call->{streamflag} ne "none") {
             print "    virStreamPtr st = NULL;\n";
             print "    daemonClientStreamPtr stream = NULL;\n";
+            print "    virNetServerProgramPtr remoteProgram = NULL;\n";
             if ($call->{sparseflag} ne "none") {
                 print "    const bool sparse = args->flags & $call->{sparseflag};\n"
             } else {
@@ -1037,6 +1038,7 @@ elsif ($mode eq "server") {
             print "    if (!(st = virStreamNew(priv->conn, VIR_STREAM_NONBLOCK)))\n";
             print "        goto cleanup;\n";
             print "\n";
+            print "    remoteProgram = virNetServerGetProgram(server, msg);\n";
             print "    if (!(stream = daemonCreateClientStream(client, st, remoteProgram, &msg->header, sparse)))\n";
             print "        goto cleanup;\n";
             print "\n";
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 4cd42ad7fd40..b1ff4a0cd751 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -205,6 +205,29 @@ virNetServerGetProgramLocked(virNetServerPtr srv,
 }
+/**
+ * virNetServerGetProgram:
+ * @srv: server (must NOT be locked by the caller)
+ * @msg: message
+ *
+ * Searches @srv for the right program for a given message @msg.
+ *
+ * Returns a pointer to the server program or NULL if not found.
+ */
+virNetServerProgramPtr
+virNetServerGetProgram(virNetServerPtr srv,
+                       virNetMessagePtr msg)
+{
+    virNetServerProgramPtr ret = NULL;
+
+    virObjectLock(srv);
+    ret = virNetServerGetProgramLocked(srv, msg);
+    virObjectUnlock(srv);
+
+    return ret;
+}
+
+
 static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
                                           virNetMessagePtr msg,
                                           void *opaque)
diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
index a79c39fdb2e7..1867e46664ba 100644
--- a/src/rpc/virnetserver.h
+++ b/src/rpc/virnetserver.h
@@ -76,6 +76,8 @@ int virNetServerSetTLSContext(virNetServerPtr srv,
                               virNetTLSContextPtr tls);
 # endif
+virNetServerProgramPtr virNetServerGetProgram(virNetServerPtr srv,
+                                              virNetMessagePtr msg);
int virNetServerAddClient(virNetServerPtr srv,
                           virNetServerClientPtr client);


    

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

John Ferlan