Re: [Qemu-devel] [PATCH v3] qemu-char: Do not disconnect when there's data for reading

[Zifei Tong]

Friendly ping.

I've saw a couple of scripts affected by this bug.

Thanks,
Zifei

On Sep 19, 2014 10:03 PM, "Kirill Batuzov" <address@hidden> wrote:

On 19.09.2014 11:12, Zifei Tong wrote:

After commit 812c1057f6175ac9a9829fa2920a2b5783814193 (Handle G_IO_HUP
in tcp_chr_read for tcp chardev), connections are disconnected when in
G_IO_HUP condition.

However, it's possible that there is still data for reading in the channel.
In that case, the remaining data is not handled.

I saw a related bug when running socat in write-only mode, after

   $ echo "quit" | socat -u - UNIX-CONNECT:qemu-monitor

the monitor won't not run the 'quit' command.

Instead of GIOCondition, this patch uses the return value of tcp_chr_recv()
to check the state of connection as suggested by Kirill.

Cc: Kirill Batuzov <address@hidden>
Cc: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
Cc: Markus Armbruster <address@hidden>
Cc: Anthony Liguori <address@hidden>
Signed-off-by: Zifei Tong <address@hidden>
---
Changes in v3: handle EWOULDBLOCK, remove inaccurate comment

  qemu-char.c | 10 ++--------
  1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 2a3cb9f..d1893a0 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2692,12 +2692,6 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
      uint8_t buf[READ_BUF_LEN];
      int len, size;

-    if (cond & G_IO_HUP) {
-        /* connection closed */
-        tcp_chr_disconnect(chr);
-        return TRUE;
-    }
-
      if (!s->connected || s->max_size <= 0) {
          return TRUE;
      }
@@ -2705,8 +2699,8 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
      if (len > s->max_size)
          len = s->max_size;
      size = tcp_chr_recv(chr, (void *)buf, len);
-    if (size == 0) {
-        /* connection closed */
+    if (size == 0 ||
+        (size < 0 && !(errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR))) {
          tcp_chr_disconnect(chr);
      } else if (size > 0) {
          if (s->do_telnetopt)

Looks good to me.

Reviewed-by: Kirill Batuzov <address@hidden>

--
Kirill