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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list