Re: [libvirt] [PATCH v3 6/6] remote/rpc: Use virNetServerGetProgram() to determine the program
On Thu, Nov 14, 2019 at 09:20 AM +0100, Pavel Hrdina 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
>> 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
>>
>> […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 >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
Re: [libvirt] [PATCH v3 6/6] remote/rpc: Use virNetServerGetProgram() to determine the program
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
> 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
>
> […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 >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
signature.asc
Description: PGP signature
--
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
On Wed, Nov 13, 2019 at 09:52 AM +0100, Pavel Hrdina 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
[…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 >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
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
> ---
> 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 >lock. This has to happen after we
acquire that lock.
Pavel
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 6/6] remote/rpc: Use virNetServerGetProgram() to determine the program
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
---
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(>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))) {
+
