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.
One of the things I want todo, however, is to add glib style signal
handling into virObject. This would enable use to safely handle the
above situation in the future.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list