[libvirt] [PATCH 35/41] remote: change hand written methods to not directly access connection

[Daniel P. Berrangé]

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.

> The same is true for
> 
>   remoteDispatchConnectDomainEventDeregister()
>   remoteDispatchConnectDomainEventRegisterAny()
>   remoteDispatchConnectDomainEventDeregisterAny()
>   remoteDispatchConnectDomainEventCallbackRegisterAny()
>   remoteDispatchConnectDomainEventCallbackDeregisterAny()
>   remoteDispatchConnectNetworkEventRegisterAny()
>   remoteDispatchConnectNetworkEventDeregisterAny()
>   remoteDispatchConnectStoragePoolEventRegisterAny()
>   remoteDispatchConnectStoragePoolEventDeregisterAny()
>   remoteDispatchConnectNodeDeviceEventRegisterAny()
>   remoteDispatchConnectNodeDeviceEventDeregisterAny()
>   remoteDispatchConnectSecretEventRegisterAny()
>   remoteDispatchConnectSecretEventDeregisterAny()
>   qemuDispatchConnectDomainMonitorEventRegister()
>   qemuDispatchConnectDomainMonitorEventDeregister()
> 
> With either all of them updated, if my understanding of the situation
> as described above is correct, or left as is otherwise,
> 
>   Reviewed-by: Andrea Bolognani <abologna at redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|