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

[Jiri Denemark]

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