On Mon, 2019-07-29 at 14:36 +0100, Daniel P. Berrangé wrote:
On Sun, Jul 28, 2019 at 08:19:40PM +0200, Andrea Bolognani wrote:
On Tue, 2019-07-23 at 17:03 +0100, Daniel P. Berrangé wrote: [...]
+++ b/src/remote/remote_daemon_dispatch.c @@ -4210,14 +4128,13 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server ATTRIBUTE_UNUSED daemonClientEventCallbackPtr ref; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); - - if (!priv->conn) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); - goto cleanup; - } + virConnectPtr conn = remoteGetHypervisorConn(client);
virMutexLock(&priv->lock);
+ if (!conn) + goto cleanup; +
Shouldn't this be *before* the virMutexLock() call? As far as I can tell, that would match the existing behavior...
Looking at this I think the original code is broken. The "cleanup:" label calls virMutexUnlock(). So the original code was jumping to the cleanup label with an unlocked mutex and then unlocking it again.
Yeah, I thought the same but I'm not too familiar with this part of libvirt. If the existing code is wrong, then I think we should have a preparatory patch addressing the issue and only replace direct struct member access with use of the newly-introduced helper function in this one. What do you think? -- Andrea Bolognani / Red Hat / Virtualization