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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
