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

[Daniel P. Berrange]

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