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

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.