[libvirt] [PATCH] ServerClient: Flush SASL data
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 /* Count of messages in the 'tx' queue, @@ -103,6 +105,7 @@ struct _virNetServerClient static void virNetServerClientDispatchEvent(virNetSocketPtr sock, int events, void *opaque); static void virNetServerClientUpdateEvent(virNetServerClientPtr client); +static void virNetServerClientDispatchRead(virNetServerClientPtr client); static void virNetServerClientLock(virNetServerClientPtr client) { @@ -191,6 +194,19 @@ static int virNetServerClientRegisterEvent(virNetServerClientPtr client) return 0; } +static void virNetServerClientDispatchReadTimerFunc(int timer, + void *opaque) +{ + virNetServerClientPtr client = opaque; + virNetServerClientLock(client); + virEventUpdateTimeout(timer, -1); + /* Even if this timer is enabled iff client->rx != NULL, + * but meanwhile client was unlocked, so it might have changed */ + if (client->rx) + virNetServerClientDispatchRead(client); + virNetServerClientUnlock(client); +} + /* * @client: a locked client object */ @@ -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 && + client->rx && + virNetSocketHasCachedData(client->sock)) { + if (client->sasl_timer) + virEventUpdateTimeout(client->sasl_timer, 0); + else + client->sasl_timer = virEventAddTimeout(0, + virNetServerClientDispatchReadTimerFunc, + client, + NULL); + } +#endif } @@ -556,6 +585,8 @@ void virNetServerClientFree(virNetServerClientPtr client) VIR_FREE(client->identity); #if HAVE_SASL virNetSASLSessionFree(client->sasl); + if (client->sasl_timer) + virEventRemoveTimeout(client->sasl_timer); #endif virNetTLSSessionFree(client->tls); virNetTLSContextFree(client->tlsCtxt); @@ -990,7 +1021,8 @@ virNetServerClientDispatchEvent(virNetSocketPtr sock, int events, void *opaque) } else { if (events & VIR_EVENT_HANDLE_WRITABLE) virNetServerClientDispatchWrite(client); - if (events & VIR_EVENT_HANDLE_READABLE) + if (events & VIR_EVENT_HANDLE_READABLE && + client->rx) virNetServerClientDispatchRead(client); } } -- 1.7.3.4
On Tue, Nov 01, 2011 at 14:14:54 +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 ... @@ -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 && + client->rx && + virNetSocketHasCachedData(client->sock)) { + if (client->sasl_timer) + virEventUpdateTimeout(client->sasl_timer, 0); + else + client->sasl_timer = virEventAddTimeout(0, + virNetServerClientDispatchReadTimerFunc, + client, + NULL); + } +#endif
Is it all really SASL-only? I think we should remove all the ifdefs (and rename sasl_timer to something like cached_timer) to enable the code everytime virNetSocketHasCachedData returns true. The thing that SASL is currently the only layer that caches data (which may not even be true, I haven't checked), it may change in the future, e.g., when adding libssh2 transport. Jirka
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 :|
From: Michal Privoznik <mprivozn@redhat.com> 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 | 29 ++++++++++++++++++++++++++++- 1 files changed, 28 insertions(+), 1 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 1256f0f..2f5ae8f 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -72,6 +72,8 @@ struct _virNetServerClient #if HAVE_SASL virNetSASLSessionPtr sasl; #endif + int sockTimer; /* Timer to be fired upon cached data, + * so we jump out from poll() immediately */ /* Count of messages in the 'tx' queue, * and the server worker pool queue @@ -103,6 +105,7 @@ struct _virNetServerClient static void virNetServerClientDispatchEvent(virNetSocketPtr sock, int events, void *opaque); static void virNetServerClientUpdateEvent(virNetServerClientPtr client); +static void virNetServerClientDispatchRead(virNetServerClientPtr client); static void virNetServerClientLock(virNetServerClientPtr client) { @@ -204,6 +207,9 @@ static void virNetServerClientUpdateEvent(virNetServerClientPtr client) mode = virNetServerClientCalculateHandleMode(client); virNetSocketUpdateIOCallback(client->sock, mode); + + if (client->rx && virNetSocketHasCachedData(client->sock)) + virEventUpdateTimeout(client->sockTimer, 0); } @@ -293,6 +299,19 @@ virNetServerClientCheckAccess(virNetServerClientPtr client) return 0; } +static void virNetServerClientSockTimerFunc(int timer, + void *opaque) +{ + virNetServerClientPtr client = opaque; + virNetServerClientLock(client); + virEventUpdateTimeout(timer, -1); + /* Although client->rx != NULL when this timer is enabled, it might have + * changed since the client was unlocked in the meantime. */ + if (client->rx) + virNetServerClientDispatchRead(client); + virNetServerClientUnlock(client); +} + virNetServerClientPtr virNetServerClientNew(virNetSocketPtr sock, int auth, @@ -319,6 +338,11 @@ virNetServerClientPtr virNetServerClientNew(virNetSocketPtr sock, client->tlsCtxt = tls; client->nrequests_max = nrequests_max; + client->sockTimer = virEventAddTimeout(-1, virNetServerClientSockTimerFunc, + client, NULL); + if (client->sockTimer < 0) + goto error; + if (tls) virNetTLSContextRef(tls); @@ -557,6 +581,8 @@ void virNetServerClientFree(virNetServerClientPtr client) #if HAVE_SASL virNetSASLSessionFree(client->sasl); #endif + if (client->sockTimer > 0) + virEventRemoveTimeout(client->sockTimer); virNetTLSSessionFree(client->tls); virNetTLSContextFree(client->tlsCtxt); virNetSocketFree(client->sock); @@ -990,7 +1016,8 @@ virNetServerClientDispatchEvent(virNetSocketPtr sock, int events, void *opaque) } else { if (events & VIR_EVENT_HANDLE_WRITABLE) virNetServerClientDispatchWrite(client); - if (events & VIR_EVENT_HANDLE_READABLE) + if (events & VIR_EVENT_HANDLE_READABLE && + client->rx) virNetServerClientDispatchRead(client); } } -- 1.7.7.1
On Tue, Nov 01, 2011 at 03:46:06PM +0100, Jiri Denemark wrote:
From: Michal Privoznik <mprivozn@redhat.com>
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 | 29 ++++++++++++++++++++++++++++- 1 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 1256f0f..2f5ae8f 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -72,6 +72,8 @@ struct _virNetServerClient #if HAVE_SASL virNetSASLSessionPtr sasl; #endif + int sockTimer; /* Timer to be fired upon cached data, + * so we jump out from poll() immediately */
/* Count of messages in the 'tx' queue, * and the server worker pool queue @@ -103,6 +105,7 @@ struct _virNetServerClient
static void virNetServerClientDispatchEvent(virNetSocketPtr sock, int events, void *opaque); static void virNetServerClientUpdateEvent(virNetServerClientPtr client); +static void virNetServerClientDispatchRead(virNetServerClientPtr client);
static void virNetServerClientLock(virNetServerClientPtr client) { @@ -204,6 +207,9 @@ static void virNetServerClientUpdateEvent(virNetServerClientPtr client) mode = virNetServerClientCalculateHandleMode(client);
virNetSocketUpdateIOCallback(client->sock, mode); + + if (client->rx && virNetSocketHasCachedData(client->sock)) + virEventUpdateTimeout(client->sockTimer, 0); }
@@ -293,6 +299,19 @@ virNetServerClientCheckAccess(virNetServerClientPtr client) return 0; }
+static void virNetServerClientSockTimerFunc(int timer, + void *opaque) +{ + virNetServerClientPtr client = opaque; + virNetServerClientLock(client); + virEventUpdateTimeout(timer, -1); + /* Although client->rx != NULL when this timer is enabled, it might have + * changed since the client was unlocked in the meantime. */ + if (client->rx) + virNetServerClientDispatchRead(client); + virNetServerClientUnlock(client); +} +
virNetServerClientPtr virNetServerClientNew(virNetSocketPtr sock, int auth, @@ -319,6 +338,11 @@ virNetServerClientPtr virNetServerClientNew(virNetSocketPtr sock, client->tlsCtxt = tls; client->nrequests_max = nrequests_max;
+ client->sockTimer = virEventAddTimeout(-1, virNetServerClientSockTimerFunc, + client, NULL); + if (client->sockTimer < 0) + goto error; + if (tls) virNetTLSContextRef(tls);
@@ -557,6 +581,8 @@ void virNetServerClientFree(virNetServerClientPtr client) #if HAVE_SASL virNetSASLSessionFree(client->sasl); #endif + if (client->sockTimer > 0) + virEventRemoveTimeout(client->sockTimer); virNetTLSSessionFree(client->tls); virNetTLSContextFree(client->tlsCtxt); virNetSocketFree(client->sock); @@ -990,7 +1016,8 @@ virNetServerClientDispatchEvent(virNetSocketPtr sock, int events, void *opaque) } else { if (events & VIR_EVENT_HANDLE_WRITABLE) virNetServerClientDispatchWrite(client); - if (events & VIR_EVENT_HANDLE_READABLE) + if (events & VIR_EVENT_HANDLE_READABLE && + client->rx) virNetServerClientDispatchRead(client); } }
ACK this looks good 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 :|
On Tue, Nov 01, 2011 at 14:49:46 +0000, Daniel P. Berrange wrote:
On Tue, Nov 01, 2011 at 03:46:06PM +0100, Jiri Denemark wrote:
From: Michal Privoznik <mprivozn@redhat.com>
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 | 29 ++++++++++++++++++++++++++++- 1 files changed, 28 insertions(+), 1 deletions(-) ... ACK this looks good
Pushed, thanks, Jirka
participants (3)
-
Daniel P. Berrange -
Jiri Denemark -
Michal Privoznik