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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
> ---
> 
> 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);
> 

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux