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

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.

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