[libvirt] [PATCH] ServerClient: Flush SASL data

[Daniel P. Berrange]

On Tue, Nov 01, 2011 at 02:14:54PM +0100, Michal Privoznik wrote:
> If daemon is using SASL it reads client data into a cache. This cache is
> big (usually 65KB) and can thus contain 2 or more messages. However,
> on socket event we can dispatch only one message. So if we read two
> messages at once, the second will not be dispatched as the socket event
> goes away with filling the cache.
> Moreover, when dispatching the cache we need to remember to take care
> of client max requests limit.
> ---
>  src/rpc/virnetserverclient.c |   34 +++++++++++++++++++++++++++++++++-
>  1 files changed, 33 insertions(+), 1 deletions(-)
> 
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> index 1256f0f..69af746 100644
> --- a/src/rpc/virnetserverclient.c
> +++ b/src/rpc/virnetserverclient.c
> @@ -71,6 +71,8 @@ struct _virNetServerClient
>      virNetTLSSessionPtr tls;
>  #if HAVE_SASL
>      virNetSASLSessionPtr sasl;
> +    int sasl_timer; /* Timer to be fired upon cached data,
> +                       so we jump out from poll() immediately */
>  #endif

Agreed with other comments that this shouldn't be #if HAVE_SASL
protected - we might someday hit this in other cases. Just call
it sockTimer or something like that.


> @@ -204,6 +220,19 @@ static void virNetServerClientUpdateEvent(virNetServerClientPtr client)
>      mode = virNetServerClientCalculateHandleMode(client);
>  
>      virNetSocketUpdateIOCallback(client->sock, mode);
> +#ifdef HAVE_SASL
> +    if (client->nrequests < client->nrequests_max &&

This line is bogus.  We set  client->rx to non-NULL, if-and-only-if
client->nrequests < client->nrequests_max. We don't want to duplicate
this check here.

> +        client->rx &&
> +        virNetSocketHasCachedData(client->sock)) {
> +        if (client->sasl_timer)
> +            virEventUpdateTimeout(client->sasl_timer, 0);
> +        else
> +            client->sasl_timer = virEventAddTimeout(0,
> +                                                    virNetServerClientDispatchReadTimerFunc,
> +                                                    client,
> +                                                    NULL);
> +    }
> +#endif

Missing a check for failure of virEventAddTimeout. I would have
unconditionally setup the timer with period -1, during the constructor
virNetServerClientNew. Then we can just use virEventUpdateTimeout
which is guaranteed to not fail.

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