On Mon, Jul 30, 2012 at 10:08:27 +0100, Daniel P. Berrange wrote:
> On Fri, Jul 27, 2012 at 06:20:04PM +0200, Jiri Denemark wrote:
> > On Fri, Jul 27, 2012 at 11:39:55 +0100, Daniel P. Berrange wrote:
> > > From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
> > >
> > > Update the remote driver to use the virNetClient close callback
> > > to trigger the virConnectPtr close callbacks
> > >
> > > Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> > > ---
> > > src/datatypes.h | 2 ++
> > > src/libvirt.c | 3 ++-
> > > src/remote/remote_driver.c | 31 +++++++++++++++++++++++++++++++
> > > 3 files changed, 35 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/datatypes.h b/src/datatypes.h
> > > index 8ac9171..a303cdc 100644
> > > --- a/src/datatypes.h
> > > +++ b/src/datatypes.h
> > > @@ -191,6 +191,8 @@ struct _virConnect {
> > > virConnectCloseFunc closeCallback;
> > > void *closeOpaque;
> > > virFreeCallback closeFreeCallback;
> > > + bool closeDispatch;
> > > + unsigned closeUnregisterCount;
> > >
> > > int refs; /* reference count */
> > > };
> > > diff --git a/src/libvirt.c b/src/libvirt.c
> > > index 160ace7..d352cfd 100644
> > > --- a/src/libvirt.c
> > > +++ b/src/libvirt.c
> > > @@ -18711,7 +18711,8 @@ int virConnectUnregisterCloseCallback(virConnectPtr conn,
> > > }
> > >
> > > conn->closeCallback = NULL;
> > > - if (conn->closeFreeCallback)
> > > + conn->closeUnregisterCount++;
> > > + if (!conn->closeDispatch && conn->closeFreeCallback)
> > > conn->closeFreeCallback(conn->closeOpaque);
> > > conn->closeFreeCallback = NULL;
> > > conn->closeOpaque = NULL;
> > > diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> > > index a69e69a..9ac27b2 100644
> > > --- a/src/remote/remote_driver.c
> > > +++ b/src/remote/remote_driver.c
> > > @@ -320,6 +320,32 @@ enum virDrvOpenRemoteFlags {
> > > };
> > >
> > >
> > > +static void remoteClientCloseFunc(virNetClientPtr client ATTRIBUTE_UNUSED,
> > > + int reason,
> > > + void *opaque)
> > > +{
> > > + virConnectPtr conn = opaque;
> > > +
> > > + virMutexLock(&conn->lock);
> > > + if (conn->closeCallback) {
> > > + virConnectCloseFunc closeCallback = conn->closeCallback;
> > > + void *closeOpaque = conn->closeOpaque;
> > > + virFreeCallback closeFreeCallback = conn->closeFreeCallback;
> > > + unsigned closeUnregisterCount = conn->closeUnregisterCount;
> > > +
> > > + VIR_DEBUG("Triggering connection close callback %p reason=%d",
> > > + conn->closeCallback, reason);
> > > + conn->closeDispatch = true;
> > > + virMutexUnlock(&conn->lock);
> > > + closeCallback(conn, reason, closeOpaque);
> > > + virMutexLock(&conn->lock);
> > > + conn->closeDispatch = false;
> > > + if (conn->closeUnregisterCount != closeUnregisterCount)
> > > + closeFreeCallback(closeOpaque);
> > > + }
> > > + virMutexUnlock(&conn->lock);
> > > +}
> > > +
> > > /*
> > > * URIs that this driver needs to handle:
> > > *
> >
> > Looks good. However, do we need to protect against the following scenario:
> >
> > - remoteClientCloseFunc locks conn, remembers stuff, unlocks conn
> > - virConnectUnregisterCloseCallback
> > - virConnectRegisterCloseCallback
> > - virConnectUnregisterCloseCallback
> > - remoteClientCloseFunc finishes
> >
> > Now the second registered callback won't be unregistered. It doesn't seem like
> > something that could happen in the real world, though.
>
> I doubt anyone will ever call UnregisterCloseCallback, let alone try
> to register a new one.
Yeah, I doubt it too. Good thing is, we won't crash in that case and the
possible leak is just client-side. I think this is good enough for now, ACK.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list