[Qemu-devel] Commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev, broke CloudStack

[Qemu-devel] Commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev, broke CloudStack

[Nils Carlson]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1052 bytes --]

Hi,

The commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev, 
broke CloudStack. CloudStack was relying on fire-and-forget style 
messaging across a unix socket to the VM. Because the host "fires" the 
message and then closes the socket a HUP is present on the line when the 
VM starts reading the socket. Commit 812c1057f ensured that the socket was 
checked for a HUP prior to calling recv, causing recv never to be called 
by the VM and no data to be read.

I've posted a patch, attached here, which moves the HUP detection to after 
all data has been read, but only for Linux as I suspect windows requires 
HUPs to be detected prior to reading data.

Could you comment on the validity of this assumption? I would be really 
happy to have this issue solved as it stops us from upgrading to later 
versions of qemu.

Amit also has concerns regarding the return values from the tcp_chr_read 
function, which seem a bit odd as they are all TRUE, even for failure 
paths.

All feedback very much appreciated.

Best Regards,
Nils Carlson



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: TEXT/x-diff; name="qemu-char:_Fix_missed_data_on_unix_socket.patch", Size: 1826 bytes --]

From pyssling@ludd.ltu.se Thu Jul 16 01:01:31 2015
Date: Wed, 15 Jul 2015 23:00:23 +0000
From: pyssling@ludd.ltu.se
To: pbonzini@redhat.com, qemu-devel@nongnu.org
Cc: Nils Carlson <pyssling@ludd.ltu.se>
Subject: [Qemu-devel] [PATCH v2] qemu-char: Fix missed data on unix socket

From: Nils Carlson <pyssling@ludd.ltu.se>

Commit 812c1057 introduced HUP detection on unix and tcp sockets prior
to a read in tcp_chr_read. This unfortunately broke CloudStack 4.2
which relied on the old behaviour where data on a socket was readable
even if a HUP was present.

On Linux a working solution seems to be to simply check the HUP after
reading all available data, i.e. recv returns a negative value,
while keeping the previous behaviour for Windows as it is known to
work.

Signed-off-by: Nils Carlson <pyssling@ludd.ltu.se>
---
 qemu-char.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qemu-char.c b/qemu-char.c
index 617e034..1e9895e 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2847,11 +2847,13 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     uint8_t buf[READ_BUF_LEN];
     int len, size;
 
+#ifdef _WIN32
     if (cond & G_IO_HUP) {
         /* connection closed */
         tcp_chr_disconnect(chr);
         return TRUE;
     }
+#endif
 
     if (!s->connected || s->max_size <= 0) {
         return TRUE;
@@ -2860,7 +2862,7 @@ 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) {
+    if (size == 0 || (size < 0 && (cond & G_IO_HUP))) {
         /* connection closed */
         tcp_chr_disconnect(chr);
     } else if (size > 0) {
-- 
1.7.10.4

Re: [Qemu-devel] Commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev, broke CloudStack

[Paolo Bonzini]

On 17/07/2015 00:51, Nils Carlson wrote:
> 
> The commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev,
> broke CloudStack. CloudStack was relying on fire-and-forget style
> messaging across a unix socket to the VM. Because the host "fires" the
> message and then closes the socket a HUP is present on the line when the
> VM starts reading the socket. Commit 812c1057f ensured that the socket
> was checked for a HUP prior to calling recv, causing recv never to be
> called by the VM and no data to be read.
> 
> I've posted a patch, attached here, which moves the HUP detection to
> after all data has been read, but only for Linux as I suspect windows
> requires HUPs to be detected prior to reading data.

I'm not sure, but I don't think this is the case.  Why do you think
Windows has this requirement?  In any case, you should prepare a patch
that has no Windows-specific paths and Cc Kirill Batuzov
(batuzovk@ispras.ru) for him to test the patch.

Alternatively I or you could test under Wine.

> Amit also has concerns regarding the return values from the tcp_chr_read
> function, which seem a bit odd as they are all TRUE, even for failure
> paths.

This is okay, I think, because the source is removed in tcp_chr_disconnect.

Paolo

Re: [Qemu-devel] Commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev, broke CloudStack

[Kirill Batuzov]

On Fri, 17 Jul 2015, Nils Carlson wrote:

> Hi,
> 
> The commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev, broke
> CloudStack. CloudStack was relying on fire-and-forget style messaging across a
> unix socket to the VM. Because the host "fires" the message and then closes
> the socket a HUP is present on the line when the VM starts reading the socket.
> Commit 812c1057f ensured that the socket was checked for a HUP prior to
> calling recv, causing recv never to be called by the VM and no data to be
> read.
> 
> I've posted a patch, attached here, which moves the HUP detection to after all
> data has been read, but only for Linux as I suspect windows requires HUPs to
> be detected prior to reading data.
> 
> Could you comment on the validity of this assumption? I would be really happy
> to have this issue solved as it stops us from upgrading to later versions of
> qemu.

I do not think your assumption is valid. Original goal of commit 812c1057f was
to handle all conditions in one watch because glib implementation for
Windows does not support multiple watches on one channel. Any changes
regarding order in which conditions are checked were unintended. On the
other hand I do not know if in pre-812c1057f implementation (with
multiple watches) this order was something defined, implementation
defined or undefined.

Some time ago another solution for this problem was proposed but was
never commited unfortunately (slipped through the cracks?).

[PATCH v3] qemu-char: Do not disconnect when there's data for reading
https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg03857.html

My comments on why I think it's better to handle disconnects with POSIX
return values can be found in the discussion of the first version of the
patch above.

https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg03261.html

Can you verify that the above patch v3 solves your problem? I would really
prefer to use return values instead of GIOConditions. They are much more
reliable and better documented.

>
> Amit also has concerns regarding the return values from the tcp_chr_read
> function, which seem a bit odd as they are all TRUE, even for failure paths.
> 
> All feedback very much appreciated.
> 
> Best Regards,
> Nils Carlson
> 
> 
> 

Re: [Qemu-devel] Commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev, broke CloudStack

[Nils Carlson]

On Fri, 17 Jul 2015, Kirill Batuzov wrote:
> On Fri, 17 Jul 2015, Nils Carlson wrote:
>
>> Hi,
>>
>> The commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev, broke
>> CloudStack. CloudStack was relying on fire-and-forget style messaging across a
>> unix socket to the VM. Because the host "fires" the message and then closes
>> the socket a HUP is present on the line when the VM starts reading the socket.
>> Commit 812c1057f ensured that the socket was checked for a HUP prior to
>> calling recv, causing recv never to be called by the VM and no data to be
>> read.
>>
>> I've posted a patch, attached here, which moves the HUP detection to after all
>> data has been read, but only for Linux as I suspect windows requires HUPs to
>> be detected prior to reading data.
>>
>> Could you comment on the validity of this assumption? I would be really happy
>> to have this issue solved as it stops us from upgrading to later versions of
>> qemu.
>
> I do not think your assumption is valid. Original goal of commit 812c1057f was
> to handle all conditions in one watch because glib implementation for
> Windows does not support multiple watches on one channel. Any changes
> regarding order in which conditions are checked were unintended. On the
> other hand I do not know if in pre-812c1057f implementation (with
> multiple watches) this order was something defined, implementation
> defined or undefined.
>
> Some time ago another solution for this problem was proposed but was
> never commited unfortunately (slipped through the cracks?).
>
> [PATCH v3] qemu-char: Do not disconnect when there's data for reading
> https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg03857.html
>
> My comments on why I think it's better to handle disconnects with POSIX
> return values can be found in the discussion of the first version of the
> patch above.
>
> https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg03261.html
>
> Can you verify that the above patch v3 solves your problem? I would really
> prefer to use return values instead of GIOConditions. They are much more
> reliable and better documented.

Sorry, didn't see your e-mail until now, so I just posted version 3 of my 
patch.

Yes, I can try the other patch as well, just give me some time.

/Nils

>>
>> Amit also has concerns regarding the return values from the tcp_chr_read
>> function, which seem a bit odd as they are all TRUE, even for failure paths.
>>
>> All feedback very much appreciated.
>>
>> Best Regards,
>> Nils Carlson
>>
>>
>>
>