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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list