[libvirt] [PATCH 1/2] RPC: Don't accept client if it would overcommit max_clients

[Daniel P. Berrange]

On Thu, Jul 25, 2013 at 04:43:52PM +0200, Michal Privoznik wrote:
> On 25.07.2013 16:37, Daniel P. Berrange wrote:
> > On Thu, Jul 25, 2013 at 04:23:32PM +0200, Michal Privoznik wrote:
> >> Currently, even if max_client limit is hit, we accept() incoming
> >> connection request, but close it immediately. This has disadvantage of
> >> not using listen() queue. We should accept() only those clients we
> >> know we can serve and let all other wait in the (limited) queue.
> >> ---
> >>  src/rpc/virnetserver.c        | 40 ++++++++++++++++++++++++++++++++++++++++
> >>  src/rpc/virnetserverservice.c |  9 +++++++++
> >>  src/rpc/virnetserverservice.h |  4 ++++
> >>  3 files changed, 53 insertions(+)
> 
> 
> >>  
> >> +    if (svc->dispatchCheckFunc &&
> >> +        svc->dispatchCheckFunc(svc, sock, svc->dispatchOpaque) < 0) {
> >> +        /* Accept declined */
> >> +        goto cleanup;
> >> +    }
> >> +
> > 
> > Rather than having this callback, can we not simply change virNetServerAddClientb()
> > to call virNetServerUpdateServices(srv, false); when a new client causes us to hit
> > the max limit ?
> > 
> 
> No, because that callback is called *after* accept() which I am trying
> to avoid.

I'm not sure I see wha the problem with doing that is.

IIUC, with your patch, if we have max clients == 30, and have 30 current
active clients, and a 31st comes in, virNetServerServiceAccept is
invoked. We then call dispatchCheckFunc see that we're at the limit,
don't accept() the connection and disable the event polling from then
on.

If we gave responsibility to the virNetServerAddClient callback, then
if we have max clients == 30, and then 30th client comes in, we accept
that, call virNetServerAddClient(), which sees we're at the limti and
so disables event polling.

So its the difference between disabling event polling at the time the
max client is accepted, or disabling it when the max + 1 client arrives
and is about to be accepted. IMHO the former makes more sense.

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 :|