[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: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(+)
> 
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index cb770c3..7ea1707 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -315,6 +315,34 @@ static int virNetServerDispatchNewClient(virNetServerServicePtr svc,
>  }
>  
>  
> +static int
> +virNetServerDispatchCheck(virNetServerServicePtr svc,
> +                          virNetSocketPtr socket,
> +                          void *opaque)
> +{
> +    virNetServerPtr srv = opaque;
> +    int ret = -1;
> +
> +    VIR_DEBUG("svc=%p socket=%p opaque=%p", svc, socket, opaque);
> +
> +    if (srv->nclients >= srv->nclients_max) {
> +        VIR_DEBUG("Not accepting client now due to max_clients setting (%zu)",
> +                  srv->nclients_max);
> +
> +        /* We need to temporarily disable the server services in order to
> +         * prevent event loop calling us over and over again. */
> +
> +        virNetServerUpdateServices(srv, false);
> +
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    return ret;
> +}
> +
> +
>  static void
>  virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED,
>                          void *context ATTRIBUTE_UNUSED)
> @@ -981,6 +1009,7 @@ int virNetServerAddService(virNetServerPtr srv,
>  
>      virNetServerServiceSetDispatcher(svc,
>                                       virNetServerDispatchNewClient,
> +                                     virNetServerDispatchCheck,
>                                       srv);
>  
>      virObjectUnlock(srv);
> @@ -1110,6 +1139,8 @@ void virNetServerRun(virNetServerPtr srv)
>                  virNetServerClientClose(srv->clients[i]);
>              if (virNetServerClientIsClosed(srv->clients[i])) {
>                  virNetServerClientPtr client = srv->clients[i];
> +                bool update_services;
> +
>                  if (srv->nclients > 1) {
>                      memmove(srv->clients + i,
>                              srv->clients + i + 1,
> @@ -1120,7 +1151,16 @@ void virNetServerRun(virNetServerPtr srv)
>                      srv->nclients = 0;
>                  }
>  
> +                /* Enable services if we can accept a new client.
> +                 * The new client can be accepted if we are at the limit. */
> +                update_services = srv->nclients == srv->nclients_max - 1;
> +
>                  virObjectUnlock(srv);
> +                if (update_services) {
> +                    /* Now it makes sense to accept() a new client. */
> +                    VIR_DEBUG("Re-enabling services");
> +                    virNetServerUpdateServices(srv, true);
> +                }
>                  virObjectUnref(client);
>                  virObjectLock(srv);
>  
> diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
> index 632f03d..a47c8f8 100644
> --- a/src/rpc/virnetserverservice.c
> +++ b/src/rpc/virnetserverservice.c
> @@ -46,6 +46,7 @@ struct _virNetServerService {
>  #endif
>  
>      virNetServerServiceDispatchFunc dispatchFunc;
> +    virNetServerServiceDispatchCheckFunc dispatchCheckFunc;
>      void *dispatchOpaque;
>  };
>  
> @@ -74,6 +75,12 @@ static void virNetServerServiceAccept(virNetSocketPtr sock,
>      virNetServerServicePtr svc = opaque;
>      virNetSocketPtr clientsock = NULL;
>  
> +    if (svc->dispatchCheckFunc &&
> +        svc->dispatchCheckFunc(svc, sock, svc->dispatchOpaque) < 0) {
> +        /* Accept declined */
> +        goto cleanup;
> +    }
> +

Rather than having this callback, can we not simply change virNetServerAddClient()
to call virNetServerUpdateServices(srv, false); when a new client causes us to hit
the max limit ?

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