* [Qemu-devel] [PATCH v3] qemu-char: Do not disconnect when there's data for reading
@ 2014-09-19 7:12 Zifei Tong
2014-09-19 14:03 ` Kirill Batuzov
2014-10-20 8:24 ` Markus Armbruster
0 siblings, 2 replies; 6+ messages in thread
From: Zifei Tong @ 2014-09-19 7:12 UTC (permalink / raw)
To: qemu-devel
Cc: Kirill Batuzov, Anthony Liguori, Markus Armbruster,
Nikolay Nikolaev, 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 <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)
--
2.1.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v3] qemu-char: Do not disconnect when there's data for reading
2014-09-19 7:12 [Qemu-devel] [PATCH v3] qemu-char: Do not disconnect when there's data for reading Zifei Tong
@ 2014-09-19 14:03 ` Kirill Batuzov
2014-10-16 11:54 ` Zifei Tong
2014-10-20 8:24 ` Markus Armbruster
1 sibling, 1 reply; 6+ messages in thread
From: Kirill Batuzov @ 2014-09-19 14:03 UTC (permalink / raw)
To: Zifei Tong, qemu-devel
Cc: Markus Armbruster, Nikolay Nikolaev, Anthony Liguori
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v3] qemu-char: Do not disconnect when there's data for reading
2014-09-19 14:03 ` Kirill Batuzov
@ 2014-10-16 11:54 ` Zifei Tong
0 siblings, 0 replies; 6+ messages in thread
From: Zifei Tong @ 2014-10-16 11:54 UTC (permalink / raw)
To: Kirill Batuzov
Cc: Markus Armbruster, qemu-devel, Nikolay Nikolaev, Anthony Liguori
[-- Attachment #1: Type: text/plain, Size: 2370 bytes --]
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
>
[-- Attachment #2: Type: text/html, Size: 3445 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] qemu-char: Do not disconnect when there's data for reading
2014-09-19 7:12 [Qemu-devel] [PATCH v3] qemu-char: Do not disconnect when there's data for reading Zifei Tong
2014-09-19 14:03 ` Kirill Batuzov
@ 2014-10-20 8:24 ` Markus Armbruster
2015-09-10 8:04 ` Michael S. Tsirkin
1 sibling, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2014-10-20 8:24 UTC (permalink / raw)
To: Zifei Tong
Cc: peter.maydell, qemu-devel, Nikolay Nikolaev, Kirill Batuzov,
Anthony Liguori, Paolo Bonzini
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)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v3] qemu-char: Do not disconnect when there's data for reading
2014-10-20 8:24 ` Markus Armbruster
@ 2015-09-10 8:04 ` Michael S. Tsirkin
2015-09-10 8:15 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2015-09-10 8:04 UTC (permalink / raw)
To: Markus Armbruster
Cc: peter.maydell, qemu-devel, Nikolay Nikolaev, Kirill Batuzov,
Anthony Liguori, Paolo Bonzini, Zifei Tong
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)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v3] qemu-char: Do not disconnect when there's data for reading
2015-09-10 8:04 ` Michael S. Tsirkin
@ 2015-09-10 8:15 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2015-09-10 8:15 UTC (permalink / raw)
To: Markus Armbruster
Cc: peter.maydell, qemu-devel, Nikolay Nikolaev, Kirill Batuzov,
Anthony Liguori, Paolo Bonzini, Zifei Tong
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)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-10 8:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-19 7:12 [Qemu-devel] [PATCH v3] qemu-char: Do not disconnect when there's data for reading Zifei Tong
2014-09-19 14:03 ` Kirill Batuzov
2014-10-16 11:54 ` Zifei Tong
2014-10-20 8:24 ` Markus Armbruster
2015-09-10 8:04 ` Michael S. Tsirkin
2015-09-10 8:15 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).