[v3] Fix virConnectRegisterCloseCallback and get rid of global variables

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

Posted by Marc Hartmayer 6 years, 3 months ago

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.ibm.com>
---
 src/libvirt_remote.syms             |   1 +
 src/remote/remote_daemon.c          |   4 +-
 src/remote/remote_daemon.h          |   2 -
 src/remote/remote_daemon_dispatch.c | 118 +++++++++++++++++++++-------
 src/rpc/gendispatch.pl              |   6 ++
 src/rpc/virnetserver.c              |  22 ++++++
 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 0493467f4603..a6883f373608 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -124,6 +124,7 @@ virNetServerGetCurrentUnauthClients;
 virNetServerGetMaxClients;
 virNetServerGetMaxUnauthClients;
 virNetServerGetName;
+virNetServerGetProgram;
 virNetServerGetThreadPoolParameters;
 virNetServerHasClients;
 virNetServerNeedsAuth;
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 7e63e180344d..c8ac224d52e9 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -73,8 +73,6 @@ VIR_LOG_INIT("daemon." DAEMON_NAME);
 #if WITH_SASL
 virNetSASLContextPtr saslCtxt = NULL;
 #endif
-virNetServerProgramPtr remoteProgram = NULL;
-virNetServerProgramPtr qemuProgram = NULL;
 
 volatile bool driversInitialized = false;
 
@@ -1007,6 +1005,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 a2d9af403619..a3d6a220f868 100644
--- a/src/remote/remote_daemon.h
+++ b/src/remote/remote_daemon.h
@@ -97,5 +97,3 @@ struct daemonClientPrivate {
 #if WITH_SASL
 extern virNetSASLContextPtr saslCtxt;
 #endif
-extern virNetServerProgramPtr remoteProgram;
-extern virNetServerProgramPtr qemuProgram;
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 70f1f7d815e8..8756bd1a222d 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -4170,9 +4170,9 @@ remoteDispatchNodeDeviceGetParent(virNetServerPtr server G_GNUC_UNUSED,
 }
 
 static int
-remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server,
                                            virNetServerClientPtr client,
-                                           virNetMessagePtr msg G_GNUC_UNUSED,
+                                           virNetMessagePtr msg,
                                            virNetMessageErrorPtr rerr)
 {
     int rv = -1;
@@ -4180,6 +4180,12 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
     virConnectPtr conn = remoteGetHypervisorConn(client);
+    virNetServerProgramPtr program;
+
+    if (!(program = virNetServerGetProgram(server, msg))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program found"));
+        goto cleanup;
+    }
 
     virMutexLock(&priv->lock);
 
@@ -4190,7 +4196,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
         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;
@@ -4242,9 +4248,9 @@ remoteDispatchConnectUnregisterCloseCallback(virNetServerPtr server G_GNUC_UNUSE
 }
 
 static int
-remoteDispatchConnectDomainEventRegister(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectDomainEventRegister(virNetServerPtr server,
                                          virNetServerClientPtr client,
-                                         virNetMessagePtr msg G_GNUC_UNUSED,
+                                         virNetMessagePtr msg,
                                          virNetMessageErrorPtr rerr G_GNUC_UNUSED,
                                          remote_connect_domain_event_register_ret *ret G_GNUC_UNUSED)
 {
@@ -4255,6 +4261,12 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server G_GNUC_UNUSED,
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
     virConnectPtr conn = remoteGetHypervisorConn(client);
+    virNetServerProgramPtr program;
+
+    if (!(program = virNetServerGetProgram(server, msg))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program found"));
+        goto cleanup;
+    }
 
     virMutexLock(&priv->lock);
 
@@ -4272,7 +4284,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server G_GNUC_UNUSED,
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(remoteProgram);
+    callback->program = virObjectRef(program);
     callback->eventID = VIR_DOMAIN_EVENT_ID_LIFECYCLE;
     callback->callbackID = -1;
     callback->legacy = true;
@@ -4463,9 +4475,9 @@ remoteDispatchDomainGetState(virNetServerPtr server G_GNUC_UNUSED,
  * VIR_DRV_SUPPORTS_FEATURE(VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK),
  * and must not mix the two styles.  */
 static int
-remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server,
                                             virNetServerClientPtr client,
-                                            virNetMessagePtr msg G_GNUC_UNUSED,
+                                            virNetMessagePtr msg,
                                             virNetMessageErrorPtr rerr G_GNUC_UNUSED,
                                             remote_connect_domain_event_register_any_args *args)
 {
@@ -4476,6 +4488,12 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
     virConnectPtr conn = remoteGetHypervisorConn(client);
+    virNetServerProgramPtr program;
+
+    if (!(program = virNetServerGetProgram(server, msg))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program found"));
+        goto cleanup;
+    }
 
     virMutexLock(&priv->lock);
 
@@ -4501,7 +4519,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(remoteProgram);
+    callback->program = virObjectRef(program);
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     callback->legacy = true;
@@ -4537,9 +4555,9 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED
 
 
 static int
-remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server,
                                                     virNetServerClientPtr client,
-                                                    virNetMessagePtr msg G_GNUC_UNUSED,
+                                                    virNetMessagePtr msg,
                                                     virNetMessageErrorPtr rerr G_GNUC_UNUSED,
                                                     remote_connect_domain_event_callback_register_any_args *args,
                                                     remote_connect_domain_event_callback_register_any_ret *ret)
@@ -4552,6 +4570,12 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server G_GNU
         virNetServerClientGetPrivateData(client);
     virDomainPtr dom = NULL;
     virConnectPtr conn = remoteGetHypervisorConn(client);
+    virNetServerProgramPtr program;
+
+    if (!(program = virNetServerGetProgram(server, msg))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program found"));
+        goto cleanup;
+    }
 
     virMutexLock(&priv->lock);
 
@@ -4577,7 +4601,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server G_GNU
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(remoteProgram);
+    callback->program = virObjectRef(program);
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     ref = callback;
@@ -5647,7 +5671,7 @@ remoteDispatchDomainMigratePrepare3Params(virNetServerPtr server G_GNUC_UNUSED,
 }
 
 static int
-remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr server,
                                                 virNetServerClientPtr client,
                                                 virNetMessagePtr msg,
                                                 virNetMessageErrorPtr rerr,
@@ -5662,6 +5686,7 @@ remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr server G_GNUC_UN
     virStreamPtr st = NULL;
     daemonClientStreamPtr stream = NULL;
     virConnectPtr conn = remoteGetHypervisorConn(client);
+    virNetServerProgramPtr program;
 
     if (!conn)
         goto cleanup;
@@ -5678,8 +5703,13 @@ remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr server G_GNUC_UN
                                   0, &params, &nparams) < 0)
         goto cleanup;
 
+    if (!(program = virNetServerGetProgram(server, msg))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program found"));
+        goto cleanup;
+    }
+
     if (!(st = virStreamNew(conn, VIR_STREAM_NONBLOCK)) ||
-        !(stream = daemonCreateClientStream(client, st, remoteProgram,
+        !(stream = daemonCreateClientStream(client, st, program,
                                             &msg->header, false)))
         goto cleanup;
 
@@ -6020,9 +6050,9 @@ static int remoteDispatchDomainCreateWithFiles(virNetServerPtr server G_GNUC_UNU
 
 
 static int
-remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server,
                                              virNetServerClientPtr client,
-                                             virNetMessagePtr msg G_GNUC_UNUSED,
+                                             virNetMessagePtr msg,
                                              virNetMessageErrorPtr rerr G_GNUC_UNUSED,
                                              remote_connect_network_event_register_any_args *args,
                                              remote_connect_network_event_register_any_ret *ret)
@@ -6035,6 +6065,12 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server G_GNUC_UNUSE
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
     virConnectPtr conn = remoteGetNetworkConn(client);
+    virNetServerProgramPtr program;
+
+    if (!(program = virNetServerGetProgram(server, msg))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program found"));
+        goto cleanup;
+    }
 
     virMutexLock(&priv->lock);
 
@@ -6060,7 +6096,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server G_GNUC_UNUSE
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(remoteProgram);
+    callback->program = virObjectRef(program);
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     ref = callback;
@@ -6141,9 +6177,9 @@ remoteDispatchConnectNetworkEventDeregisterAny(virNetServerPtr server G_GNUC_UNU
 }
 
 static int
-remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server,
                                                  virNetServerClientPtr client,
-                                                 virNetMessagePtr msg G_GNUC_UNUSED,
+                                                 virNetMessagePtr msg,
                                                  virNetMessageErrorPtr rerr G_GNUC_UNUSED,
                                                  remote_connect_storage_pool_event_register_any_args *args,
                                                  remote_connect_storage_pool_event_register_any_ret *ret)
@@ -6156,6 +6192,12 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server G_GNUC_U
         virNetServerClientGetPrivateData(client);
     virStoragePoolPtr  pool = NULL;
     virConnectPtr conn = remoteGetStorageConn(client);
+    virNetServerProgramPtr program;
+
+    if (!(program = virNetServerGetProgram(server, msg))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program found"));
+        goto cleanup;
+    }
 
     virMutexLock(&priv->lock);
 
@@ -6181,7 +6223,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server G_GNUC_U
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(remoteProgram);
+    callback->program = virObjectRef(program);
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     ref = callback;
@@ -6261,9 +6303,9 @@ remoteDispatchConnectStoragePoolEventDeregisterAny(virNetServerPtr server G_GNUC
 }
 
 static int
-remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server,
                                                 virNetServerClientPtr client,
-                                                virNetMessagePtr msg G_GNUC_UNUSED,
+                                                virNetMessagePtr msg,
                                                 virNetMessageErrorPtr rerr G_GNUC_UNUSED,
                                                 remote_connect_node_device_event_register_any_args *args,
                                                 remote_connect_node_device_event_register_any_ret *ret)
@@ -6276,6 +6318,12 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server G_GNUC_UN
         virNetServerClientGetPrivateData(client);
     virNodeDevicePtr  dev = NULL;
     virConnectPtr conn = remoteGetNodeDevConn(client);
+    virNetServerProgramPtr program;
+
+    if (!(program = virNetServerGetProgram(server, msg))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program found"));
+        goto cleanup;
+    }
 
     virMutexLock(&priv->lock);
 
@@ -6301,7 +6349,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server G_GNUC_UN
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(remoteProgram);
+    callback->program = virObjectRef(program);
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     ref = callback;
@@ -6381,9 +6429,9 @@ remoteDispatchConnectNodeDeviceEventDeregisterAny(virNetServerPtr server G_GNUC_
 }
 
 static int
-remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server,
                                             virNetServerClientPtr client,
-                                            virNetMessagePtr msg G_GNUC_UNUSED,
+                                            virNetMessagePtr msg,
                                             virNetMessageErrorPtr rerr G_GNUC_UNUSED,
                                             remote_connect_secret_event_register_any_args *args,
                                             remote_connect_secret_event_register_any_ret *ret)
@@ -6396,6 +6444,12 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED
         virNetServerClientGetPrivateData(client);
     virSecretPtr secret = NULL;
     virConnectPtr conn = remoteGetSecretConn(client);
+    virNetServerProgramPtr program;
+
+    if (!(program = virNetServerGetProgram(server, msg))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program found"));
+        goto cleanup;
+    }
 
     virMutexLock(&priv->lock);
 
@@ -6421,7 +6475,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(remoteProgram);
+    callback->program = virObjectRef(program);
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     ref = callback;
@@ -6501,9 +6555,9 @@ remoteDispatchConnectSecretEventDeregisterAny(virNetServerPtr server G_GNUC_UNUS
 }
 
 static int
-qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server G_GNUC_UNUSED,
+qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server,
                                               virNetServerClientPtr client,
-                                              virNetMessagePtr msg G_GNUC_UNUSED,
+                                              virNetMessagePtr msg,
                                               virNetMessageErrorPtr rerr G_GNUC_UNUSED,
                                               qemu_connect_domain_monitor_event_register_args *args,
                                               qemu_connect_domain_monitor_event_register_ret *ret)
@@ -6517,6 +6571,12 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server G_GNUC_UNUS
     virDomainPtr dom = NULL;
     const char *event = args->event ? *args->event : NULL;
     virConnectPtr conn = remoteGetHypervisorConn(client);
+    virNetServerProgramPtr program;
+
+    if (!(program = virNetServerGetProgram(server, msg))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program found"));
+        goto cleanup;
+    }
 
     virMutexLock(&priv->lock);
 
@@ -6536,7 +6596,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server G_GNUC_UNUS
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
     callback->client = virObjectRef(client);
-    callback->program = virObjectRef(qemuProgram);
+    callback->program = virObjectRef(program);
     callback->eventID = -1;
     callback->callbackID = -1;
     ref = callback;
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index 7c868191d19c..2b8795beec16 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -1052,6 +1052,7 @@ elsif ($mode eq "server") {
         if ($call->{streamflag} ne "none") {
             print "    virStreamPtr st = NULL;\n";
             print "    daemonClientStreamPtr stream = NULL;\n";
+            print "    virNetServerProgramPtr remoteProgram;\n";
             if ($call->{sparseflag} ne "none") {
                 print "    const bool sparse = args->flags & $call->{sparseflag};\n"
             } else {
@@ -1093,6 +1094,11 @@ elsif ($mode eq "server") {
             print "    if (!(st = virStreamNew($conn_var, VIR_STREAM_NONBLOCK)))\n";
             print "        goto cleanup;\n";
             print "\n";
+            print "    if (!(remoteProgram = virNetServerGetProgram(server, msg))) {\n";
+            print "        virReportError(VIR_ERR_INTERNAL_ERROR, \"%s\", _(\"no matching program found\"));\n";
+            print "        goto cleanup;\n";
+            print "    }\n";
+            print "\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 154747a1a097..80a28123c536 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -192,6 +192,28 @@ virNetServerGetProgramLocked(virNetServerPtr srv,
     return NULL;
 }
 
+/**
+ * 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;
+
+    virObjectLock(srv);
+    ret = virNetServerGetProgramLocked(srv, msg);
+    virObjectUnlock(srv);
+
+    return ret;
+}
+
 static void
 virNetServerDispatchNewMessage(virNetServerClientPtr client,
                                virNetMessagePtr msg,
diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
index 260c99b22d5e..46ecb0e91077 100644
--- a/src/rpc/virnetserver.h
+++ b/src/rpc/virnetserver.h
@@ -90,6 +90,8 @@ int virNetServerAddProgram(virNetServerPtr srv,
 int virNetServerSetTLSContext(virNetServerPtr srv,
                               virNetTLSContextPtr tls);
 
+virNetServerProgramPtr virNetServerGetProgram(virNetServerPtr srv,
+                                              virNetMessagePtr msg);
 
 int virNetServerAddClient(virNetServerPtr srv,
                           virNetServerClientPtr client);
-- 
2.21.0

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

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

Posted by Pavel Hrdina 6 years, 2 months ago

On Fri, Nov 01, 2019 at 06:35:48PM +0100, 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.ibm.com>
> ---
>  src/libvirt_remote.syms             |   1 +
>  src/remote/remote_daemon.c          |   4 +-
>  src/remote/remote_daemon.h          |   2 -
>  src/remote/remote_daemon_dispatch.c | 118 +++++++++++++++++++++-------
>  src/rpc/gendispatch.pl              |   6 ++
>  src/rpc/virnetserver.c              |  22 ++++++
>  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 0493467f4603..a6883f373608 100644
> --- a/src/libvirt_remote.syms
> +++ b/src/libvirt_remote.syms
> @@ -124,6 +124,7 @@ virNetServerGetCurrentUnauthClients;
>  virNetServerGetMaxClients;
>  virNetServerGetMaxUnauthClients;
>  virNetServerGetName;
> +virNetServerGetProgram;
>  virNetServerGetThreadPoolParameters;
>  virNetServerHasClients;
>  virNetServerNeedsAuth;
> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> index 7e63e180344d..c8ac224d52e9 100644
> --- a/src/remote/remote_daemon.c
> +++ b/src/remote/remote_daemon.c
> @@ -73,8 +73,6 @@ VIR_LOG_INIT("daemon." DAEMON_NAME);
>  #if WITH_SASL
>  virNetSASLContextPtr saslCtxt = NULL;
>  #endif
> -virNetServerProgramPtr remoteProgram = NULL;
> -virNetServerProgramPtr qemuProgram = NULL;
>  
>  volatile bool driversInitialized = false;
>  
> @@ -1007,6 +1005,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 a2d9af403619..a3d6a220f868 100644
> --- a/src/remote/remote_daemon.h
> +++ b/src/remote/remote_daemon.h
> @@ -97,5 +97,3 @@ struct daemonClientPrivate {
>  #if WITH_SASL
>  extern virNetSASLContextPtr saslCtxt;
>  #endif
> -extern virNetServerProgramPtr remoteProgram;
> -extern virNetServerProgramPtr qemuProgram;
> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
> index 70f1f7d815e8..8756bd1a222d 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -4170,9 +4170,9 @@ remoteDispatchNodeDeviceGetParent(virNetServerPtr server G_GNUC_UNUSED,
>  }
>  
>  static int
> -remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
> +remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server,
>                                             virNetServerClientPtr client,
> -                                           virNetMessagePtr msg G_GNUC_UNUSED,
> +                                           virNetMessagePtr msg,
>                                             virNetMessageErrorPtr rerr)
>  {
>      int rv = -1;
> @@ -4180,6 +4180,12 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
>      struct daemonClientPrivate *priv =
>          virNetServerClientGetPrivateData(client);
>      virConnectPtr conn = remoteGetHypervisorConn(client);
> +    virNetServerProgramPtr program;
> +
> +    if (!(program = virNetServerGetProgram(server, msg))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program found"));
> +        goto cleanup;
> +    }

This doesn't look right.  If the function fails we will jump to cleanup
where we will try to unlock &priv->lock.  This has to happen after we
acquire that lock.

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

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

Posted by Marc Hartmayer 6 years, 2 months ago

On Wed, Nov 13, 2019 at 09:52 AM +0100, Pavel Hrdina <phrdina@redhat.com> wrote:
> On Fri, Nov 01, 2019 at 06:35:48PM +0100, 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.ibm.com>

[…snip…]

>>                                             virNetMessageErrorPtr rerr)
>>  {
>>      int rv = -1;
>> @@ -4180,6 +4180,12 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
>>      struct daemonClientPrivate *priv =
>>          virNetServerClientGetPrivateData(client);
>>      virConnectPtr conn = remoteGetHypervisorConn(client);
>> +    virNetServerProgramPtr program;
>> +
>> +    if (!(program = virNetServerGetProgram(server, msg))) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program found"));
>> +        goto cleanup;
>> +    }
>
> This doesn't look right.  If the function fails we will jump to cleanup
> where we will try to unlock &priv->lock.  This has to happen after we
> acquire that lock.
>
> Pavel

Yep, will fix that as well. Shall I directly return in the error case or
jump to another label (e.g. 'cleanup_unlock')?

Or do see any reason why we should hold the priv->lock during the
virNetServerGetProgram call?

-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
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 v3 6/6] remote/rpc: Use virNetServerGetProgram() to determine the program

Posted by Pavel Hrdina 6 years, 2 months ago

On Wed, Nov 13, 2019 at 07:12:34PM +0100, Marc Hartmayer wrote:
> On Wed, Nov 13, 2019 at 09:52 AM +0100, Pavel Hrdina <phrdina@redhat.com> wrote:
> > On Fri, Nov 01, 2019 at 06:35:48PM +0100, 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.ibm.com>
> 
> […snip…]
> 
> >>                                             virNetMessageErrorPtr rerr)
> >>  {
> >>      int rv = -1;
> >> @@ -4180,6 +4180,12 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
> >>      struct daemonClientPrivate *priv =
> >>          virNetServerClientGetPrivateData(client);
> >>      virConnectPtr conn = remoteGetHypervisorConn(client);
> >> +    virNetServerProgramPtr program;
> >> +
> >> +    if (!(program = virNetServerGetProgram(server, msg))) {
> >> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program found"));
> >> +        goto cleanup;
> >> +    }
> >
> > This doesn't look right.  If the function fails we will jump to cleanup
> > where we will try to unlock &priv->lock.  This has to happen after we
> > acquire that lock.
> >
> > Pavel
> 
> Yep, will fix that as well. Shall I directly return in the error case or
> jump to another label (e.g. 'cleanup_unlock')?

Returning directly would not work properly as well, we call
virNetMessageSaveError() in case of error in the cleanup section.

Another label would work.

> Or do see any reason why we should hold the priv->lock during the
> virNetServerGetProgram call?

We don't have to hold the lock for virNetServerGetProgram(), it just
makes the function easier to follow as there will be only one cleanup
and I don't see any harm of holding the lock for that function call as
well if the function will later wait for the same lock.

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

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

Posted by Marc Hartmayer 6 years, 2 months ago

On Thu, Nov 14, 2019 at 09:20 AM +0100, Pavel Hrdina <phrdina@redhat.com> wrote:
> On Wed, Nov 13, 2019 at 07:12:34PM +0100, Marc Hartmayer wrote:
>> On Wed, Nov 13, 2019 at 09:52 AM +0100, Pavel Hrdina <phrdina@redhat.com> wrote:
>> > On Fri, Nov 01, 2019 at 06:35:48PM +0100, 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.ibm.com>
>> 
>> […snip…]
>> 
>> >>                                             virNetMessageErrorPtr rerr)
>> >>  {
>> >>      int rv = -1;
>> >> @@ -4180,6 +4180,12 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
>> >>      struct daemonClientPrivate *priv =
>> >>          virNetServerClientGetPrivateData(client);
>> >>      virConnectPtr conn = remoteGetHypervisorConn(client);
>> >> +    virNetServerProgramPtr program;
>> >> +
>> >> +    if (!(program = virNetServerGetProgram(server, msg))) {
>> >> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program found"));
>> >> +        goto cleanup;
>> >> +    }
>> >
>> > This doesn't look right.  If the function fails we will jump to cleanup
>> > where we will try to unlock &priv->lock.  This has to happen after we
>> > acquire that lock.
>> >
>> > Pavel
>> 
>> Yep, will fix that as well. Shall I directly return in the error case or
>> jump to another label (e.g. 'cleanup_unlock')?
>
> Returning directly would not work properly as well, we call
> virNetMessageSaveError() in case of error in the cleanup section.
>
> Another label would work.
>
>> Or do see any reason why we should hold the priv->lock during the
>> virNetServerGetProgram call?
>
> We don't have to hold the lock for virNetServerGetProgram(), it just
> makes the function easier to follow as there will be only one cleanup
> and I don't see any harm of holding the lock for that function call as
> well if the function will later wait for the same lock.

This would enforce the lock order 'server -> priv->lock' (not sure if
this is already the case) + it will become harder to identify what we’re
trying to protect with the lock. So if you have no strong opinion about
that I will introduce a 'cleanup_unlock' label. Fine with you?

Thanks.

>
> Pavel
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
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

[libvirt] [PATCH v3 1/6] rpc: use the return value of virObjectRef directly
[libvirt] [PATCH v3 2/6] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
[libvirt] [PATCH v3 3/6] remote: Save reference to program in daemonClientEventCallback
[libvirt] [PATCH v3 4/6] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
[libvirt] [PATCH v3 5/6] rpc: Introduce virNetServerGetProgramLocked helper function
[libvirt] [PATCH v3 6/6] remote/rpc: Use virNetServerGetProgram() to determine the program