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

[Andrea Bolognani]

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