[libvirt] [PATCH v2 03/12] Introduce two public APIs for keepalive protocol

[Jiri Denemark]

On Mon, Oct 03, 2011 at 10:42:30 +0100, Daniel P. Berrange wrote:
> On Mon, Oct 03, 2011 at 10:26:30AM +0100, Daniel P. Berrange wrote:
> > On Fri, Sep 23, 2011 at 10:24:48AM +0200, Jiri Denemark wrote:
> > > This introduces virConnectAllowKeepAlive and virConnectStartKeepAlive
> > > public APIs which can be used by a client connecting to remote server to
> > > indicate support for keepalive protocol. Both APIs are handled directly
> > > by remote driver and not transmitted over the wire to the server.
> > > ---
> > >  include/libvirt/libvirt.h.in |    5 ++
> > >  src/driver.h                 |    9 ++++
> > >  src/libvirt.c                |  107 ++++++++++++++++++++++++++++++++++++++++++
> > >  src/libvirt_internal.h       |   10 +++-
> > >  src/libvirt_public.syms      |    6 ++
> > >  5 files changed, 135 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> > > index 39155a6..6f61cc0 100644
> > > --- a/include/libvirt/libvirt.h.in
> > > +++ b/include/libvirt/libvirt.h.in
> > > @@ -3210,6 +3210,11 @@ typedef struct _virTypedParameter virMemoryParameter;
> > >   */
> > >  typedef virMemoryParameter *virMemoryParameterPtr;
> > >  
> > > +int virConnectAllowKeepAlive(virConnectPtr conn);
> > > +int virConnectStartKeepAlive(virConnectPtr conn,
> > > +                             int interval,
> > > +                             unsigned int count);
> > 
> > With this design, even if both the client and server support
> > keepalive at a protocol level, it will only ever be enabled if
> > the client application remembers to call virConnectAllowKeepAlive.
> > I think this puts too much responsibility on the client, at the
> > detriment of the server.
> > 
> > An administrator of libvirtd might want to mandate use of keep
> > alive for all clients (knowing this would exclude any libvirt
> > client <= 0.9.6 of course). With this design they cannot do this
> > since they are reliant on the client application programmer to
> > call virConnectAllowKeepAlive, which I believe 95% of people will
> > never bother todo.
> > 
> > IMHO we should change this so that
> > 
> >  - In remote_driver.c, doRemoteOpen(), after performing
> >    authentication, but before opening the connection
> >    we should send a
> > 
> >       supports_feature(KEEPALIVE)
> > 
> >  - Upon receiving supports_feature(KEEPALIVE) the server
> >    shall be free to start sending keep alives, if it is
> >    configured todo so.
> 
> 
> Hmm, actually I see elsewhere that responding to server initiated
> pings, requires help of the event loop which we can't assume is
> running.
> 
> I guess we could enable it if we detect that an event loop has been
> registered.  This would make us hit the virsh bug again where we
> register the event loop, but never run it. Arguably that bug should
> be fixed in virsh for other reasons, so perhaps this is justification
> enough todo so.

Right, we could do that once the bug in virsh is fixed so I'll try to
incorporate the virsh fix into this series.

> If only we had made use of an event loop mandatory all those years
> ago :-(

Oh yes, client-side implementation would be much more simple without the buck
passing algorithm. Although that would mean significantly less fun on that
part :-)

Jirka