Re: [libvirt] [PATCH 01/11] Process all pending I/O for a RPC client before checking EOF

On Thu, Jul 26, 2012 at 04:00:22PM +0200, Jiri Denemark wrote: > On Tue, Jul 24, 2012 at 14:22:43 +0100, Daniel P. Berrange wrote: > > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > > > In the socket event handler for the RPC client we must deal > > with read/write events, before checking for EOF, otherwise > > we might close the socket before we've read & acted upon the > > last RPC messages > > > > Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; > > --- > > src/rpc/virnetclient.c | 20 ++++++++++---------- > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c > > index aba58ec..f5d8189 100644 > > --- a/src/rpc/virnetclient.c > > +++ b/src/rpc/virnetclient.c > > @@ -1694,16 +1694,6 @@ void virNetClientIncomingEvent(virNetSocketPtr sock, > > > > VIR_DEBUG("Event fired %p %d", sock, events); > > > > - if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR)) { > > - VIR_DEBUG("%s : VIR_EVENT_HANDLE_HANGUP or " > > - "VIR_EVENT_HANDLE_ERROR encountered", __FUNCTION__); > > - virNetClientMarkClose(client, > > - (events & VIR_EVENT_HANDLE_HANGUP) ? > > - VIR_CONNECT_CLOSE_REASON_EOF : > > - VIR_CONNECT_CLOSE_REASON_ERROR); > > - goto done; > > - } > > - > > if (events & VIR_EVENT_HANDLE_WRITABLE) { > > if (virNetClientIOHandleOutput(client) < 0) > > virNetClientMarkClose(client, VIR_CONNECT_CLOSE_REASON_ERROR); > > @@ -1714,6 +1704,16 @@ void virNetClientIncomingEvent(virNetSocketPtr sock, > > virNetClientMarkClose(client, VIR_CONNECT_CLOSE_REASON_ERROR); > > } > > > > + if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR)) { > > + VIR_DEBUG("%s : VIR_EVENT_HANDLE_HANGUP or " > > + "VIR_EVENT_HANDLE_ERROR encountered", __FUNCTION__); > > I know you are just copy&pasting but __FUNCTION__ is redundant here. > > > + virNetClientMarkClose(client, > > + (events & VIR_EVENT_HANDLE_HANGUP) ? > > + VIR_CONNECT_CLOSE_REASON_EOF : > > + VIR_CONNECT_CLOSE_REASON_ERROR); > > + goto done; > > + } > > + > > /* Remove completed calls or signal their threads. */ > > virNetClientCallRemovePredicate(&client->waitDispatch, > > virNetClientIOEventLoopRemoveDone, > > This is a good change but I think it's incomplete. If something fails (e.g. > the connection is closed) while we are inside virNetClientIOHandleOutput(), we > will still skip reading any data possibly waiting for us. No, we should be ok in that respect, since we see the error, but don't 'goto', so we'll still run the input code if (events & VIR_EVENT_HANDLE_WRITABLE) { if (virNetClientIOHandleOutput(client) < 0) virNetClientMarkClose(client, VIR_CONNECT_CLOSE_REASON_ERROR); } if (events & VIR_EVENT_HANDLE_READABLE) { if (virNetClientIOHandleInput(client) < 0) virNetClientMarkClose(client, VIR_CONNECT_CLOSE_REASON_ERROR); }