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

[Qemu-devel] [PATCH] 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), the connection is disconnected when in
G_IO_HUP condition.

However, it's possible that the channel is in G_IO_IN condition at the
same time, meaning there is data for reading. In that case, the
remaining data is not handled.

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

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

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

CC: Kirill Batuzov <batuzovk@ispras.ru>
CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
CC: Anthony Liguori <aliguori@amazon.com>
Signed-off-by: Zifei Tong <zifeitong@gmail.com>
---
 qemu-char.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-char.c b/qemu-char.c
index 1a8d9aa..5018c3a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2706,7 +2706,7 @@ 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) {
+    if (!(cond & G_IO_IN) && (cond & G_IO_HUP)) {
         /* connection closed */
         tcp_chr_disconnect(chr);
         return TRUE;
-- 
2.1.0

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

[Zifei Tong]

Friendly ping.

Also CC to qemu-trivial.

On Sun, Sep 7, 2014 at 8:24 PM, Zifei Tong <zifeitong@gmail.com> wrote:
> After commit 812c1057f6175ac9a9829fa2920a2b5783814193 (Handle G_IO_HUP
> in tcp_chr_read for tcp chardev), the connection is disconnected when in
> G_IO_HUP condition.
>
> However, it's possible that the channel is in G_IO_IN condition at the
> same time, meaning there is data for reading. In that case, the
> remaining data is not handled.
>
> I saw a related bug when running socat in write-only mode, with
>
>   $ echo "quit" | socat -u - UNIX-CONNECT:qemu-monitor
>
> the monitor won't not run the 'quit' command.
>
> CC: Kirill Batuzov <batuzovk@ispras.ru>
> CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> CC: Anthony Liguori <aliguori@amazon.com>
> Signed-off-by: Zifei Tong <zifeitong@gmail.com>
> ---
>  qemu-char.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 1a8d9aa..5018c3a 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2706,7 +2706,7 @@ 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) {
> +    if (!(cond & G_IO_IN) && (cond & G_IO_HUP)) {
>          /* connection closed */
>          tcp_chr_disconnect(chr);
>          return TRUE;
> --
> 2.1.0
>

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

[Michael Tokarev]

16.09.2014 09:31, Zifei Tong wrote:
> Friendly ping.
> 
> Also CC to qemu-trivial.

There was at least 2 subsequent versions of this down the line,
a v2 and a v3.  (Just to show why I haven't applied it to -trivial).

Thanks,

/mjt

> On Sun, Sep 7, 2014 at 8:24 PM, Zifei Tong <zifeitong@gmail.com> wrote:
>> After commit 812c1057f6175ac9a9829fa2920a2b5783814193 (Handle G_IO_HUP
>> in tcp_chr_read for tcp chardev), the connection is disconnected when in
>> G_IO_HUP condition.
>>
>> However, it's possible that the channel is in G_IO_IN condition at the
>> same time, meaning there is data for reading. In that case, the
>> remaining data is not handled.
>>
>> I saw a related bug when running socat in write-only mode, with
>>
>>   $ echo "quit" | socat -u - UNIX-CONNECT:qemu-monitor
>>
>> the monitor won't not run the 'quit' command.
>>
>> CC: Kirill Batuzov <batuzovk@ispras.ru>
>> CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
>> CC: Anthony Liguori <aliguori@amazon.com>
>> Signed-off-by: Zifei Tong <zifeitong@gmail.com>
>> ---
>>  qemu-char.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 1a8d9aa..5018c3a 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2706,7 +2706,7 @@ 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) {
>> +    if (!(cond & G_IO_IN) && (cond & G_IO_HUP)) {
>>          /* connection closed */
>>          tcp_chr_disconnect(chr);
>>          return TRUE;
>> --
>> 2.1.0
>>
> 

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

[Michael S. Tsirkin]

On Wed, Sep 24, 2014 at 11:52:37AM +0400, Michael Tokarev wrote:
> 16.09.2014 09:31, Zifei Tong wrote:
> > Friendly ping.
> > 
> > Also CC to qemu-trivial.
> 
> There was at least 2 subsequent versions of this down the line,
> a v2 and a v3.  (Just to show why I haven't applied it to -trivial).
> 
> Thanks,
> 
> /mjt

It's also far from trivial.
I don't see why it applies there.

> > On Sun, Sep 7, 2014 at 8:24 PM, Zifei Tong <zifeitong@gmail.com> wrote:
> >> After commit 812c1057f6175ac9a9829fa2920a2b5783814193 (Handle G_IO_HUP
> >> in tcp_chr_read for tcp chardev), the connection is disconnected when in
> >> G_IO_HUP condition.
> >>
> >> However, it's possible that the channel is in G_IO_IN condition at the
> >> same time, meaning there is data for reading. In that case, the
> >> remaining data is not handled.
> >>
> >> I saw a related bug when running socat in write-only mode, with
> >>
> >>   $ echo "quit" | socat -u - UNIX-CONNECT:qemu-monitor
> >>
> >> the monitor won't not run the 'quit' command.
> >>
> >> CC: Kirill Batuzov <batuzovk@ispras.ru>
> >> CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> >> CC: Anthony Liguori <aliguori@amazon.com>
> >> Signed-off-by: Zifei Tong <zifeitong@gmail.com>
> >> ---
> >>  qemu-char.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/qemu-char.c b/qemu-char.c
> >> index 1a8d9aa..5018c3a 100644
> >> --- a/qemu-char.c
> >> +++ b/qemu-char.c
> >> @@ -2706,7 +2706,7 @@ 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) {
> >> +    if (!(cond & G_IO_IN) && (cond & G_IO_HUP)) {
> >>          /* connection closed */
> >>          tcp_chr_disconnect(chr);
> >>          return TRUE;
> >> --
> >> 2.1.0
> >>
> > 
> 

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

[Markus Armbruster]

Cc'ing Gerd for additional chardev expertise.

Zifei Tong <zifeitong@gmail.com> writes:

> After commit 812c1057f6175ac9a9829fa2920a2b5783814193 (Handle G_IO_HUP
> in tcp_chr_read for tcp chardev), the connection is disconnected when in
> G_IO_HUP condition.
>
> However, it's possible that the channel is in G_IO_IN condition at the
> same time, meaning there is data for reading. In that case, the
> remaining data is not handled.
>
> I saw a related bug when running socat in write-only mode, with
>
>   $ echo "quit" | socat -u - UNIX-CONNECT:qemu-monitor
>
> the monitor won't not run the 'quit' command.

Reproduced.

Is this a regression caused by command 812c105?

> CC: Kirill Batuzov <batuzovk@ispras.ru>
> CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> CC: Anthony Liguori <aliguori@amazon.com>
> Signed-off-by: Zifei Tong <zifeitong@gmail.com>
> ---
>  qemu-char.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 1a8d9aa..5018c3a 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2706,7 +2706,7 @@ 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) {
> +    if (!(cond & G_IO_IN) && (cond & G_IO_HUP)) {
>          /* connection closed */
>          tcp_chr_disconnect(chr);
>          return TRUE;

Kirill, you added the code being changed.  Could you review the patch?

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

[Zifei Tong]

On Tue, Sep 16, 2014 at 2:06 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Cc'ing Gerd for additional chardev expertise.
>
> Zifei Tong <zifeitong@gmail.com> writes:
>
>> After commit 812c1057f6175ac9a9829fa2920a2b5783814193 (Handle G_IO_HUP
>> in tcp_chr_read for tcp chardev), the connection is disconnected when in
>> G_IO_HUP condition.
>>
>> However, it's possible that the channel is in G_IO_IN condition at the
>> same time, meaning there is data for reading. In that case, the
>> remaining data is not handled.
>>
>> I saw a related bug when running socat in write-only mode, with
>>
>>   $ echo "quit" | socat -u - UNIX-CONNECT:qemu-monitor
>>
>> the monitor won't not run the 'quit' command.
>
> Reproduced.
>
> Is this a regression caused by command 812c105?

The first bad commit is cdaa86 ("Add G_IO_HUP handler for socket
chardev"), which is reverted and reimplemented by 812c105.

>> CC: Kirill Batuzov <batuzovk@ispras.ru>
>> CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
>> CC: Anthony Liguori <aliguori@amazon.com>
>> Signed-off-by: Zifei Tong <zifeitong@gmail.com>
>> ---
>>  qemu-char.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 1a8d9aa..5018c3a 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2706,7 +2706,7 @@ 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) {
>> +    if (!(cond & G_IO_IN) && (cond & G_IO_HUP)) {
>>          /* connection closed */
>>          tcp_chr_disconnect(chr);
>>          return TRUE;
>
> Kirill, you added the code being changed.  Could you review the patch?

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

[Kirill Batuzov]

On Tue, 16 Sep 2014, Markus Armbruster wrote:

> 
> Kirill, you added the code being changed.  Could you review the patch?
>

I'll try but this is more about GIOConditions which I do not understand
well. See below.

Zifei Tong <zifeitong@gmail.com> writes:

> After commit 812c1057f6175ac9a9829fa2920a2b5783814193 (Handle G_IO_HUP
> in tcp_chr_read for tcp chardev), the connection is disconnected when in
> G_IO_HUP condition.
>
> However, it's possible that the channel is in G_IO_IN condition at the
> same time, meaning there is data for reading. In that case, the
> remaining data is not handled.
>
> I saw a related bug when running socat in write-only mode, with
>
>   $ echo "quit" | socat -u - UNIX-CONNECT:qemu-monitor
>
> the monitor won't not run the 'quit' command.
> CC: Kirill Batuzov <batuzovk@ispras.ru>
> CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> CC: Anthony Liguori <aliguori@amazon.com>
> Signed-off-by: Zifei Tong <zifeitong@gmail.com>
> ---
>  qemu-char.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 1a8d9aa..5018c3a 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2706,7 +2706,7 @@ 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) {
> +    if (!(cond & G_IO_IN) && (cond & G_IO_HUP)) {
>          /* connection closed */
>          tcp_chr_disconnect(chr);
>          return TRUE;

I've tried running the above code and watching in debugger what is
happening. I wanted to know that tcp_chr_disconnect is called properly
so I replaced the 'quit' command with 'help'. From the way code works I
have a feeling that we are using some undocumented linux-specific
behaviour here.

What I saw:
 - Sometimes all three (G_IO_IN, G_IO_HUP and G_IO_ERR) conditions are
   asserted, sometimes only two of them (G_IO_IN and G_IO_HUP).
 - G_IO_IN is *never* de-asserted. Even after all data is depleted it is
   still up.
 - When G_IO_ERR is asserted and all data have been read one call to
   tcp_chr_recv returns -1. Subsequent calls return 0.

GIOCondition behaviour in corner cases is puzzling and can differ from OS
to OS (commit 812c105 is an example, there also were freebsd-specific
bugfixes if I remember correctly).

I suggest we remove condition checks completely and use more reliable
and better documented source of information - return value of
tcp_chr_recv (which is just return value of recvmsg). All we need to do
is to handle 'size < 0' and not forget about EAGAIN case.

Currently we have a mix of GLib conditions and POSIX return values to
handle all cases and we can not do it with GLib alone (I do not know a
way to tell if there is data in channel or not when G_IO_HUP is asserted).

All these problems were before this patch, but I think it is better to
fix it once than add patch over patch fighting GIOCondition's weird
behaviour.

-- 
Kirill

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

[Zifei Tong]

On Tue, Sep 16, 2014 at 11:30 PM, Kirill Batuzov <batuzovk@ispras.ru> wrote:
> On Tue, 16 Sep 2014, Markus Armbruster wrote:
>
>>
>> Kirill, you added the code being changed.  Could you review the patch?
>>
>
> I'll try but this is more about GIOConditions which I do not understand
> well. See below.
>
> Zifei Tong <zifeitong@gmail.com> writes:
>
>> After commit 812c1057f6175ac9a9829fa2920a2b5783814193 (Handle G_IO_HUP
>> in tcp_chr_read for tcp chardev), the connection is disconnected when in
>> G_IO_HUP condition.
>>
>> However, it's possible that the channel is in G_IO_IN condition at the
>> same time, meaning there is data for reading. In that case, the
>> remaining data is not handled.
>>
>> I saw a related bug when running socat in write-only mode, with
>>
>>   $ echo "quit" | socat -u - UNIX-CONNECT:qemu-monitor
>>
>> the monitor won't not run the 'quit' command.
>> CC: Kirill Batuzov <batuzovk@ispras.ru>
>> CC: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
>> CC: Anthony Liguori <aliguori@amazon.com>
>> Signed-off-by: Zifei Tong <zifeitong@gmail.com>
>> ---
>>  qemu-char.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 1a8d9aa..5018c3a 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2706,7 +2706,7 @@ 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) {
>> +    if (!(cond & G_IO_IN) && (cond & G_IO_HUP)) {
>>          /* connection closed */
>>          tcp_chr_disconnect(chr);
>>          return TRUE;

Hi Kirill,

Thanks for your review and detailed analysis.

> I've tried running the above code and watching in debugger what is
> happening. I wanted to know that tcp_chr_disconnect is called properly
> so I replaced the 'quit' command with 'help'. From the way code works I
> have a feeling that we are using some undocumented linux-specific
> behaviour here.
>
> What I saw:
>  - Sometimes all three (G_IO_IN, G_IO_HUP and G_IO_ERR) conditions are
>    asserted, sometimes only two of them (G_IO_IN and G_IO_HUP).
>  - G_IO_IN is *never* de-asserted. Even after all data is depleted it is
>    still up.
>  - When G_IO_ERR is asserted and all data have been read one call to
>    tcp_chr_recv returns -1. Subsequent calls return 0.
>
> GIOCondition behaviour in corner cases is puzzling and can differ from OS
> to OS (commit 812c105 is an example, there also were freebsd-specific
> bugfixes if I remember correctly).
>
> I suggest we remove condition checks completely and use more reliable
> and better documented source of information - return value of
> tcp_chr_recv (which is just return value of recvmsg). All we need to do
> is to handle 'size < 0' and not forget about EAGAIN case.

This sounds good to me. I'll send a V2 patch based on this.

Thanks,
Zifei

> Currently we have a mix of GLib conditions and POSIX return values to
> handle all cases and we can not do it with GLib alone (I do not know a
> way to tell if there is data in channel or not when G_IO_HUP is asserted).
>
> All these problems were before this patch, but I think it is better to
> fix it once than add patch over patch fighting GIOCondition's weird
> behaviour.
>
> --
> Kirill