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

[Daniel P. Berrange]

On Wed, Jun 13, 2012 at 01:29:18AM +0200, Jiri Denemark wrote:
> So far, we were dropping non-blocking calls whenever sending them would block.
> 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;




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