Re: [libvirt] [PATCH v3 4/5] Update remote driver to support the connection close callbacks

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(a)redhat.com&gt; > > > > Update the remote driver to use the virNetClient close callback > > to trigger the virConnectPtr close callbacks > > > > Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; > > --- > > 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.