Re: [Qemu-devel] [PATCH] qemu-char: Do not disconnect when there's data for reading
On Tue, Sep 16, 2014 at 11:30 PM, Kirill Batuzov batuz...@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 zifeit...@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 batuz...@ispras.ru
CC: Nikolay Nikolaev n.nikol...@virtualopensystems.com
CC: Anthony Liguori aligu...@amazon.com
Signed-off-by: Zifei Tong zifeit...@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
Re: [Qemu-devel] [PATCH] qemu-char: Do not disconnect when there's data for reading
Cc'ing Gerd for additional chardev expertise.
Zifei Tong zifeit...@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 batuz...@ispras.ru
CC: Nikolay Nikolaev n.nikol...@virtualopensystems.com
CC: Anthony Liguori aligu...@amazon.com
Signed-off-by: Zifei Tong zifeit...@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
On Tue, Sep 16, 2014 at 2:06 PM, Markus Armbruster arm...@redhat.com wrote:
Cc'ing Gerd for additional chardev expertise.
Zifei Tong zifeit...@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 batuz...@ispras.ru
CC: Nikolay Nikolaev n.nikol...@virtualopensystems.com
CC: Anthony Liguori aligu...@amazon.com
Signed-off-by: Zifei Tong zifeit...@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
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 zifeit...@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 batuz...@ispras.ru
CC: Nikolay Nikolaev n.nikol...@virtualopensystems.com
CC: Anthony Liguori aligu...@amazon.com
Signed-off-by: Zifei Tong zifeit...@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
Friendly ping.
Also CC to qemu-trivial.
On Sun, Sep 7, 2014 at 8:24 PM, Zifei Tong zifeit...@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 batuz...@ispras.ru
CC: Nikolay Nikolaev n.nikol...@virtualopensystems.com
CC: Anthony Liguori aligu...@amazon.com
Signed-off-by: Zifei Tong zifeit...@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
