* [Qemu-devel] Commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev, broke CloudStack
@ 2015-07-16 22:51 Nils Carlson
2015-07-17 2:22 ` Paolo Bonzini
2015-07-17 9:35 ` Kirill Batuzov
0 siblings, 2 replies; 4+ messages in thread
From: Nils Carlson @ 2015-07-16 22:51 UTC (permalink / raw)
To: Paolo Bonzini, liangshunbin, qemu-devel, batuzovk, a.motakis,
n.nikolaev, mst, zodiac, amit.shah
[-- 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] Commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev, broke CloudStack
2015-07-16 22:51 [Qemu-devel] Commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev, broke CloudStack Nils Carlson
@ 2015-07-17 2:22 ` Paolo Bonzini
2015-07-17 9:35 ` Kirill Batuzov
1 sibling, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2015-07-17 2:22 UTC (permalink / raw)
To: Nils Carlson, liangshunbin, qemu-devel, batuzovk, a.motakis,
n.nikolaev, mst, zodiac, amit.shah
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] Commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev, broke CloudStack
2015-07-16 22:51 [Qemu-devel] Commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev, broke CloudStack Nils Carlson
2015-07-17 2:22 ` Paolo Bonzini
@ 2015-07-17 9:35 ` Kirill Batuzov
2015-07-17 17:26 ` Nils Carlson
1 sibling, 1 reply; 4+ messages in thread
From: Kirill Batuzov @ 2015-07-17 9:35 UTC (permalink / raw)
To: Nils Carlson
Cc: mst, zodiac, qemu-devel, n.nikolaev, liangshunbin, a.motakis,
amit.shah, Paolo Bonzini
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
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] Commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev, broke CloudStack
2015-07-17 9:35 ` Kirill Batuzov
@ 2015-07-17 17:26 ` Nils Carlson
0 siblings, 0 replies; 4+ messages in thread
From: Nils Carlson @ 2015-07-17 17:26 UTC (permalink / raw)
To: Kirill Batuzov
Cc: mst, zodiac, qemu-devel, n.nikolaev, liangshunbin, a.motakis,
amit.shah, Paolo Bonzini
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
>>
>>
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-07-17 17:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-16 22:51 [Qemu-devel] Commit 812c1057f, Handle G_IO_HUP in tcp_chr_read for tcp chardev, broke CloudStack Nils Carlson
2015-07-17 2:22 ` Paolo Bonzini
2015-07-17 9:35 ` Kirill Batuzov
2015-07-17 17:26 ` Nils Carlson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.