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

[Michael S. Tsirkin]

On Thu, Sep 10, 2015 at 11:04:05AM +0300, Michael S. Tsirkin wrote:
> On Mon, Oct 20, 2014 at 10:24:44AM +0200, Markus Armbruster wrote:
> > MAINTAINERS points to Anthony, and you duly cc'ed him, but he's
> > effectively retired.  Cc'ing recent committers include Paolo and Peter.
> > 
> > Zifei Tong  writes:
> 
> I merged the patch that broke this, so I'll merge the fix too.

Oh, Paolo merged it already. Thanks Paolo!

> 
> > > 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 
> > > Cc: Nikolay Nikolaev 
> > > Cc: Markus Armbruster 
> > > Cc: Anthony Liguori 
> > > Signed-off-by: Zifei Tong 
> > > ---
> > > 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)

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

[Michael S. Tsirkin]

On Mon, Oct 20, 2014 at 10:24:44AM +0200, Markus Armbruster wrote:
> MAINTAINERS points to Anthony, and you duly cc'ed him, but he's
> effectively retired.  Cc'ing recent committers include Paolo and Peter.
> 
> Zifei Tong  writes:

I merged the patch that broke this, so I'll merge the fix too.


> > 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 
> > Cc: Nikolay Nikolaev 
> > Cc: Markus Armbruster 
> > Cc: Anthony Liguori 
> > Signed-off-by: Zifei Tong 
> > ---
> > 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)

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

[Markus Armbruster]

MAINTAINERS points to Anthony, and you duly cc'ed him, but he's
effectively retired.  Cc'ing recent committers include Paolo and Peter.

Zifei Tong zifeit...@gmail.com writes:

 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 batuz...@ispras.ru
 Cc: Nikolay Nikolaev n.nikol...@virtualopensystems.com
 Cc: Markus Armbruster arm...@redhat.com
 Cc: Anthony Liguori aligu...@amazon.com
 Signed-off-by: Zifei Tong zifeit...@gmail.com
 ---
 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)

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 batuz...@ispras.ru 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 batuz...@ispras.ru
 Cc: Nikolay Nikolaev n.nikol...@virtualopensystems.com
 Cc: Markus Armbruster arm...@redhat.com
 Cc: Anthony Liguori aligu...@amazon.com
 Signed-off-by: Zifei Tong zifeit...@gmail.com
 ---
 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 batuz...@ispras.ru

 --
 Kirill

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

[Zifei Tong]

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 batuz...@ispras.ru
Cc: Nikolay Nikolaev n.nikol...@virtualopensystems.com
Cc: Markus Armbruster arm...@redhat.com
Cc: Anthony Liguori aligu...@amazon.com
Signed-off-by: Zifei Tong zifeit...@gmail.com
---
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)
-- 
2.1.0

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

[Kirill Batuzov]

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 batuz...@ispras.ru
Cc: Nikolay Nikolaev n.nikol...@virtualopensystems.com
Cc: Markus Armbruster arm...@redhat.com
Cc: Anthony Liguori aligu...@amazon.com
Signed-off-by: Zifei Tong zifeit...@gmail.com
---
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 batuz...@ispras.ru

--
Kirill