Re: [libvirt] [PATCH 00/12] rpc: Fix several issues with keepalive messages

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.

On reviewing the code, I noticed the following (unrelated) issue we should fix. First 'poll' can't return EWOULDBLOCK,

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;