On Wed, Jun 13, 2012 at 10:54:02 +0100, Daniel P. Berrange wrote:
On Wed, Jun 13, 2012 at 01:29:18AM +0200, Jiri Denemark wrote:
In case a client is sending lots of stream calls (which are not supposed to generate any reply), the assumption that having other calls in a queue is sufficient to get a reply from the server doesn't work. I tried to fix this in b1e374a7ac56927cfe62435179bf0bba1e08b372 but failed and reverted that commit. While working on the proper fix, I discovered several other issues we had in handling keepalive messages in client RPC code. See individual patches for more details.
As a nice bonus, the fixed version is shorter by one line than the current broken version :-)
Ok, this refactoring actually addresses many of the concerns I had with the original design and so passes my totally subjective "feels right" test. In particular the way we handle keepalives while in the virNetClientIOEventLoop() method & use event handles while no, are the key problem areas we're fixing with this.
Thanks for the review, I tested scenarios covered by patches 2, 3, 4, 9, and 12 and all was working fine. I'll be pushing the series soon and I hope it won't generate tons of bugs as I'll be on vacation for two weeks starting from Friday :-P
On reviewing the code, I noticed the following (unrelated) issue we should fix. First 'poll' can't return EWOULDBLOCK,
Oh yeah, the code I always wanted to remove but was afraid of doing so :-)
and second, we're checking errno so far away from the poll() call that we've probably already trashed the original errno value.
Daniel
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 033fda6..49d238e 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -1347,6 +1347,12 @@ static int virNetClientIOEventLoop(virNetClientPtr client,
virNetClientLock(client);
+ if (ret < 0) { + virReportSystemError(errno, + "%s", _("poll on socket failed")); + goto error; + } + if (virKeepAliveTrigger(client->keepalive, &msg)) { client->wantClose = true; } else if (msg && virNetClientQueueNonBlocking(client, msg) < 0) { @@ -1375,15 +1381,6 @@ static int virNetClientIOEventLoop(virNetClientPtr client, } }
- if (ret < 0) { - /* XXX what's this dubious errno check doing ? */ - if (errno == EWOULDBLOCK) - continue; - virReportSystemError(errno, - "%s", _("poll on socket failed")); - goto error; - } - if (fds[0].revents & POLLOUT) { if (virNetClientIOHandleOutput(client) < 0) goto error;
ACK, I'll push this as a 13th patch in this series. Jirka