[libvirt] [PATCH] client: Report proper close reason

Jiri Denemark posted 1 patch 6 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/d5dbf4d0f3a01dca4e4719deb038f530c997e02e.1493736959.git.jdenemar@redhat.com
src/rpc/virnetclient.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
[libvirt] [PATCH] client: Report proper close reason
Posted by Jiri Denemark 6 years, 10 months ago
When we get a POLLHUP or VIR_EVENT_HANDLE_HANGUP event for a client, we
still want to read from the socket to process any accumulated data. But
doing so inevitably results in an error and a call to
virNetClientMarkClose before we get to processing the hangup event (and
another call to virNetClientMarkClose). However the close reason passed
to the second virNetClientMarkClose call is ignored because another one
was already set. We need to pass the correct close reason when marking
the socket to be closed for the first time.

https://bugzilla.redhat.com/show_bug.cgi?id=1373859

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/rpc/virnetclient.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index c95974794..837a8a707 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1595,6 +1595,7 @@ static int virNetClientIOEventLoop(virNetClientPtr client,
 {
     struct pollfd fds[2];
     bool error = false;
+    int closeReason;
     int ret;
 
     fds[0].fd = virNetSocketGetFD(client->sock);
@@ -1703,9 +1704,14 @@ static int virNetClientIOEventLoop(virNetClientPtr client,
             }
         }
 
+        if (fds[0].revents & POLLHUP)
+            closeReason = VIR_CONNECT_CLOSE_REASON_EOF;
+        else
+            closeReason = VIR_CONNECT_CLOSE_REASON_ERROR;
+
         if (fds[0].revents & POLLOUT) {
             if (virNetClientIOHandleOutput(client) < 0) {
-                virNetClientMarkClose(client, VIR_CONNECT_CLOSE_REASON_ERROR);
+                virNetClientMarkClose(client, closeReason);
                 error = true;
                 /* Fall through to process any pending data. */
             }
@@ -1713,7 +1719,7 @@ static int virNetClientIOEventLoop(virNetClientPtr client,
 
         if (fds[0].revents & POLLIN) {
             if (virNetClientIOHandleInput(client) < 0) {
-                virNetClientMarkClose(client, VIR_CONNECT_CLOSE_REASON_ERROR);
+                virNetClientMarkClose(client, closeReason);
                 error = true;
                 /* Fall through to process any pending data. */
             }
@@ -1746,13 +1752,13 @@ static int virNetClientIOEventLoop(virNetClientPtr client,
         if (fds[0].revents & POLLHUP) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("received hangup event on socket"));
-            virNetClientMarkClose(client, VIR_CONNECT_CLOSE_REASON_EOF);
+            virNetClientMarkClose(client, closeReason);
             goto error;
         }
         if (fds[0].revents & POLLERR) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("received error event on socket"));
-            virNetClientMarkClose(client, VIR_CONNECT_CLOSE_REASON_ERROR);
+            virNetClientMarkClose(client, closeReason);
             goto error;
         }
     }
@@ -1968,6 +1974,7 @@ void virNetClientIncomingEvent(virNetSocketPtr sock,
                                void *opaque)
 {
     virNetClientPtr client = opaque;
+    int closeReason;
 
     virObjectLock(client);
 
@@ -1981,23 +1988,25 @@ void virNetClientIncomingEvent(virNetSocketPtr sock,
 
     VIR_DEBUG("Event fired %p %d", sock, events);
 
+    if (events & VIR_EVENT_HANDLE_HANGUP)
+        closeReason = VIR_CONNECT_CLOSE_REASON_EOF;
+    else
+        closeReason = VIR_CONNECT_CLOSE_REASON_ERROR;
+
     if (events & VIR_EVENT_HANDLE_WRITABLE) {
         if (virNetClientIOHandleOutput(client) < 0)
-            virNetClientMarkClose(client, VIR_CONNECT_CLOSE_REASON_ERROR);
+            virNetClientMarkClose(client, closeReason);
     }
 
     if (events & VIR_EVENT_HANDLE_READABLE) {
         if (virNetClientIOHandleInput(client) < 0)
-            virNetClientMarkClose(client, VIR_CONNECT_CLOSE_REASON_ERROR);
+            virNetClientMarkClose(client, closeReason);
     }
 
     if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR)) {
         VIR_DEBUG("VIR_EVENT_HANDLE_HANGUP or "
                   "VIR_EVENT_HANDLE_ERROR encountered");
-        virNetClientMarkClose(client,
-                              (events & VIR_EVENT_HANDLE_HANGUP) ?
-                              VIR_CONNECT_CLOSE_REASON_EOF :
-                              VIR_CONNECT_CLOSE_REASON_ERROR);
+        virNetClientMarkClose(client, closeReason);
         goto done;
     }
 
-- 
2.12.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] client: Report proper close reason
Posted by Peter Krempa 6 years, 10 months ago
On Tue, May 02, 2017 at 16:55:59 +0200, Jiri Denemark wrote:
> When we get a POLLHUP or VIR_EVENT_HANDLE_HANGUP event for a client, we
> still want to read from the socket to process any accumulated data. But
> doing so inevitably results in an error and a call to
> virNetClientMarkClose before we get to processing the hangup event (and
> another call to virNetClientMarkClose). However the close reason passed
> to the second virNetClientMarkClose call is ignored because another one
> was already set. We need to pass the correct close reason when marking
> the socket to be closed for the first time.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1373859

So this basically partially overrides what this commit has done (wrong):

commit f1271380381bb49b4a4c38950fe77a60d19ea9a3
Author: Martin Kletzander <mkletzan@redhat.com>
Date:   Sun Nov 30 20:09:08 2014 +0100

    rpc: Report proper close reason
    
    Whenever client socket was marked as closed for some reason, it could've
    been changed when really closing the connection.  With this patch the
    proper reason is kept since the first time it's marked as closed.

Without the mentioned commit the last error would be remembered. It is
probable that in some cases the error would be overwritten incorrectly
so I think that the combination of these two patches should do the
trick.

ACK

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list