[Qemu-devel] [PATCH] qemu-char: Fix missed data on unix socket

[Qemu-devel] [PATCH] qemu-char: Fix missed data on unix socket

[pyssling]

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 available data, while keeping the original behaviour for windows.

There is then a divergence in behaviour for the two platforms, but this
seems better than breaking a whole software stack.

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..cfd9b70 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] [PATCH] qemu-char: Fix missed data on unix socket

[Paolo Bonzini]

On 13/07/2015 10:13, pyssling@ludd.ltu.se wrote:
> 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 available data, while keeping the original behaviour for windows.
> 
> There is then a divergence in behaviour for the two platforms, but this
> seems better than breaking a whole software stack.

There is no need to do something special on Windows, I think.  You can
unconditionally check G_IO_HUP after reading.  One nit:

> -    if (size == 0) {
> +    if (size == 0 || (size < 0 && cond & G_IO_HUP)) {

Please put (cond & G_IO_HUP) within parentheses.

Please

Re: [Qemu-devel] [PATCH] qemu-char: Fix missed data on unix socket

[Amit Shah]

On (Mon) 13 Jul 2015 [12:15:22], Paolo Bonzini wrote:
> 
> 
> On 13/07/2015 10:13, pyssling@ludd.ltu.se wrote:
> > 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 available data, while keeping the original behaviour for windows.
> > 
> > There is then a divergence in behaviour for the two platforms, but this
> > seems better than breaking a whole software stack.
> 
> There is no need to do something special on Windows, I think.  You can
> unconditionally check G_IO_HUP after reading.  One nit:
> 
> > -    if (size == 0) {
> > +    if (size == 0 || (size < 0 && cond & G_IO_HUP)) {
> 
> Please put (cond & G_IO_HUP) within parentheses.

Also, returning TRUE there isn't right - if the connection ends, we
should return FALSE.


		Amit

Re: [Qemu-devel] [PATCH] qemu-char: Fix missed data on unix socket

[Nils Carlson]

On Mon, 13 Jul 2015, Amit Shah wrote:

> On (Mon) 13 Jul 2015 [12:15:22], Paolo Bonzini wrote:
>>
>>
>> On 13/07/2015 10:13, pyssling@ludd.ltu.se wrote:
>>> 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 available data, while keeping the original behaviour for windows.
>>>
>>> There is then a divergence in behaviour for the two platforms, but this
>>> seems better than breaking a whole software stack.
>>
>> There is no need to do something special on Windows, I think.  You can
>> unconditionally check G_IO_HUP after reading.  One nit:
>>
>>> -    if (size == 0) {
>>> +    if (size == 0 || (size < 0 && cond & G_IO_HUP)) {
>>
>> Please put (cond & G_IO_HUP) within parentheses.
>
> Also, returning TRUE there isn't right - if the connection ends, we
> should return FALSE.

I agree that this seems reasonable. I will change it and re-test.

>
>
> 		Amit
>
>

Re: [Qemu-devel] [PATCH] qemu-char: Fix missed data on unix socket

[Nils Carlson]

On Mon, 13 Jul 2015, Nils Carlson wrote:

> On Mon, 13 Jul 2015, Amit Shah wrote:

<snip>

>> Also, returning TRUE there isn't right - if the connection ends, we
>> should return FALSE.
>
> I agree that this seems reasonable. I will change it and re-test.
>

I had a closer look, and it seems always returning true is intentional 
here, the called function, tcp_chr_disconnect(chr), handles the 
deregistration from handlers. If we were to return FALSE we would be 
duplicating work and possibly breaking things.

Nils

>> 
>>
>> 		Amit
>> 
>> 
>
>

Re: [Qemu-devel] [PATCH] qemu-char: Fix missed data on unix socket

[Amit Shah]

On (Wed) 15 Jul 2015 [23:44:52], Nils Carlson wrote:
> On Mon, 13 Jul 2015, Nils Carlson wrote:
> 
> >On Mon, 13 Jul 2015, Amit Shah wrote:
> 
> <snip>
> 
> >>Also, returning TRUE there isn't right - if the connection ends, we
> >>should return FALSE.
> >
> >I agree that this seems reasonable. I will change it and re-test.
> >
> 
> I had a closer look, and it seems always returning true is intentional here,
> the called function, tcp_chr_disconnect(chr), handles the deregistration
> from handlers. If we were to return FALSE we would be duplicating work and
> possibly breaking things.

Not sure how.

Anyway, can you please start a new thread, with the author and
reviewers of the patch CC'ed, so they can chime in as well?

		Amit

Re: [Qemu-devel] [PATCH] qemu-char: Fix missed data on unix socket

[Nils Carlson]

On Thu, 16 Jul 2015, Amit Shah wrote:

> On (Wed) 15 Jul 2015 [23:44:52], Nils Carlson wrote:
>> On Mon, 13 Jul 2015, Nils Carlson wrote:
>>
>>> On Mon, 13 Jul 2015, Amit Shah wrote:
>>
>> <snip>
>>
>>>> Also, returning TRUE there isn't right - if the connection ends, we
>>>> should return FALSE.
>>>
>>> I agree that this seems reasonable. I will change it and re-test.
>>>
>>
>> I had a closer look, and it seems always returning true is intentional here,
>> the called function, tcp_chr_disconnect(chr), handles the deregistration
>> from handlers. If we were to return FALSE we would be duplicating work and
>> possibly breaking things.
>
> Not sure how.
>
> Anyway, can you please start a new thread, with the author and
> reviewers of the patch CC'ed, so they can chime in as well?

The authors and reviewers of which patch exactly?

The fix for windows which broke cloudstack was done in 812c1057.
The returning TRUE everywhere was done in commit cdbf6e16 by Paolo 
Bonzini, CC:d above.

I don't think there was anything wrong with either patch, the former one 
simply broken a strange behaviour CloudStack was relying on which I would 
say is dubious (fire and forget messaging.)

/Nils

Re: [Qemu-devel] [PATCH] qemu-char: Fix missed data on unix socket

[Amit Shah]

On (Thu) 16 Jul 2015 [10:11:04], Nils Carlson wrote:
> On Thu, 16 Jul 2015, Amit Shah wrote:
> 
> >On (Wed) 15 Jul 2015 [23:44:52], Nils Carlson wrote:
> >>On Mon, 13 Jul 2015, Nils Carlson wrote:
> >>
> >>>On Mon, 13 Jul 2015, Amit Shah wrote:
> >>
> >><snip>
> >>
> >>>>Also, returning TRUE there isn't right - if the connection ends, we
> >>>>should return FALSE.
> >>>
> >>>I agree that this seems reasonable. I will change it and re-test.
> >>>
> >>
> >>I had a closer look, and it seems always returning true is intentional here,
> >>the called function, tcp_chr_disconnect(chr), handles the deregistration
> >>from handlers. If we were to return FALSE we would be duplicating work and
> >>possibly breaking things.
> >
> >Not sure how.
> >
> >Anyway, can you please start a new thread, with the author and
> >reviewers of the patch CC'ed, so they can chime in as well?
> 
> The authors and reviewers of which patch exactly?
> 
> The fix for windows which broke cloudstack was done in 812c1057.

Yea, the author of this patch.  Also, I meant the return TRUE in this
patch, not the one Paolo added.

> The returning TRUE everywhere was done in commit cdbf6e16 by Paolo Bonzini,
> CC:d above.
> 
> I don't think there was anything wrong with either patch, the former one
> simply broken a strange behaviour CloudStack was relying on which I would
> say is dubious (fire and forget messaging.)

There need not be anything wrong with a previous patch; but just
because someone touched the area recently, they might have more
context, and even help in spotting the bug.


		Amit