Re: [libvirt] [PATCH v3 36/48] remote: fix lock ordering mistake in event registration

2019-07-30 Thread Andrea Bolognani 
On Mon, 2019-07-29 at 18:11 +0100, Daniel P. Berrangé wrote:
> If the event (un)registration methods are invoked while no connection is
> open, they jump to a cleanup block which unlocks a mutex which is not
> currently locked.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/remote/remote_daemon_dispatch.c | 64 ++---
>  1 file changed, 32 insertions(+), 32 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v3 36/48] remote: fix lock ordering mistake in event registration

2019-07-30 Thread Ján Tomko 

On Mon, Jul 29, 2019 at 06:11:18PM +0100, Daniel P. Berrangé wrote:

If the event (un)registration methods are invoked while no connection is
open, they jump to a cleanup block which unlocks a mutex which is not
currently locked.

Signed-off-by: Daniel P. Berrangé 
---
src/remote/remote_daemon_dispatch.c | 64 ++---
1 file changed, 32 insertions(+), 32 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v3 36/48] remote: fix lock ordering mistake in event registration

2019-07-29 Thread Daniel P . Berrangé 
If the event (un)registration methods are invoked while no connection is
open, they jump to a cleanup block which unlocks a mutex which is not
currently locked.

Signed-off-by: Daniel P. Berrangé 
---
 src/remote/remote_daemon_dispatch.c | 64 ++---
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/src/remote/remote_daemon_dispatch.c 
b/src/remote/remote_daemon_dispatch.c
index 90103f5093..4a3312a944 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -4212,13 +4212,13 @@ 
remoteDispatchConnectDomainEventRegister(virNetServerPtr server ATTRIBUTE_UNUSED
 struct daemonClientPrivate *priv =
 virNetServerClientGetPrivateData(client);
 
+virMutexLock(>lock);
+
 if (!priv->conn) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
 goto cleanup;
 }
 
-virMutexLock(>lock);
-
 /* If we call register first, we could append a complete callback
  * to our array, but on OOM append failure, we'd have to then hope
  * deregister works to undo our register.  So instead we append an
@@ -4276,13 +4276,13 @@ 
remoteDispatchConnectDomainEventDeregister(virNetServerPtr server ATTRIBUTE_UNUS
 struct daemonClientPrivate *priv =
 virNetServerClientGetPrivateData(client);
 
+virMutexLock(>lock);
+
 if (!priv->conn) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
 goto cleanup;
 }
 
-virMutexLock(>lock);
-
 for (i = 0; i < priv->ndomainEventCallbacks; i++) {
 if (priv->domainEventCallbacks[i]->eventID == 
VIR_DOMAIN_EVENT_ID_LIFECYCLE) {
 callbackID = priv->domainEventCallbacks[i]->callbackID;
@@ -4440,13 +4440,13 @@ 
remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU
 struct daemonClientPrivate *priv =
 virNetServerClientGetPrivateData(client);
 
+virMutexLock(>lock);
+
 if (!priv->conn) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
 goto cleanup;
 }
 
-virMutexLock(>lock);
-
 /* We intentionally do not use VIR_DOMAIN_EVENT_ID_LAST here; any
  * new domain events added after this point should only use the
  * modern callback style of RPC.  */
@@ -4516,13 +4516,13 @@ 
remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server ATTRI
 virNetServerClientGetPrivateData(client);
 virDomainPtr dom = NULL;
 
+virMutexLock(>lock);
+
 if (!priv->conn) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
 goto cleanup;
 }
 
-virMutexLock(>lock);
-
 if (args->dom &&
 !(dom = get_nonnull_domain(priv->conn, *args->dom)))
 goto cleanup;
@@ -4590,13 +4590,13 @@ 
remoteDispatchConnectDomainEventDeregisterAny(virNetServerPtr server ATTRIBUTE_U
 struct daemonClientPrivate *priv =
 virNetServerClientGetPrivateData(client);
 
+virMutexLock(>lock);
+
 if (!priv->conn) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
 goto cleanup;
 }
 
-virMutexLock(>lock);
-
 /* We intentionally do not use VIR_DOMAIN_EVENT_ID_LAST here; any
  * new domain events added after this point should only use the
  * modern callback style of RPC.  */
@@ -4647,13 +4647,13 @@ 
remoteDispatchConnectDomainEventCallbackDeregisterAny(virNetServerPtr server ATT
 struct daemonClientPrivate *priv =
 virNetServerClientGetPrivateData(client);
 
+virMutexLock(>lock);
+
 if (!priv->conn) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
 goto cleanup;
 }
 
-virMutexLock(>lock);
-
 for (i = 0; i < priv->ndomainEventCallbacks; i++) {
 if (priv->domainEventCallbacks[i]->callbackID == args->callbackID)
 break;
@@ -6089,13 +6089,13 @@ 
remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server ATTRIBUTE_UN
 virNetServerClientGetPrivateData(client);
 virNetworkPtr net = NULL;
 
+virMutexLock(>lock);
+
 if (!priv->networkConn) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
 goto cleanup;
 }
 
-virMutexLock(>lock);
-
 if (args->net &&
 !(net = get_nonnull_network(priv->networkConn, *args->net)))
 goto cleanup;
@@ -6162,13 +6162,13 @@ 
remoteDispatchConnectNetworkEventDeregisterAny(virNetServerPtr server ATTRIBUTE_
 struct daemonClientPrivate *priv =
 virNetServerClientGetPrivateData(client);
 
+virMutexLock(>lock);
+
 if (!priv->networkConn) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
 goto cleanup;
 }
 
-virMutexLock(>lock);
-
 for (i = 0; i < priv->nnetworkEventCallbacks; i++) {
 if (priv->networkEventCallbacks[i]->callbackID == args->callbackID)
 break;
@@