Message ID | 1411110773-489-1-git-send-email-zifeitong@gmail.com |
---|---|
State | New |
Headers | show |
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 <batuzovk@ispras.ru> > Cc: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Anthony Liguori <aliguori@amazon.com> > Signed-off-by: Zifei Tong <zifeitong@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 <batuzovk@ispras.ru>
Friendly ping. I've saw a couple of scripts affected by this bug. Thanks, Zifei On Sep 19, 2014 10:03 PM, "Kirill Batuzov" <batuzovk@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 <batuzovk@ispras.ru> >> Cc: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> >> Cc: Markus Armbruster <armbru@redhat.com> >> Cc: Anthony Liguori <aliguori@amazon.com> >> Signed-off-by: Zifei Tong <zifeitong@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 <batuzovk@ispras.ru> > > -- > Kirill >
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 <zifeitong@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 <batuzovk@ispras.ru> > Cc: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Anthony Liguori <aliguori@amazon.com> > Signed-off-by: Zifei Tong <zifeitong@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)
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 <zifeitong@gmail.com> 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 <batuzovk@ispras.ru> > > Cc: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> > > Cc: Markus Armbruster <armbru@redhat.com> > > Cc: Anthony Liguori <aliguori@amazon.com> > > Signed-off-by: Zifei Tong <zifeitong@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)
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 <zifeitong@gmail.com> 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 <batuzovk@ispras.ru> > > > Cc: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> > > > Cc: Markus Armbruster <armbru@redhat.com> > > > Cc: Anthony Liguori <aliguori@amazon.com> > > > Signed-off-by: Zifei Tong <zifeitong@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)
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)
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 <batuzovk@ispras.ru> Cc: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> Cc: Markus Armbruster <armbru@redhat.com> Cc: Anthony Liguori <aliguori@amazon.com> Signed-off-by: Zifei Tong <zifeitong@gmail.com> --- Changes in v3: handle EWOULDBLOCK, remove inaccurate comment qemu-char.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)