[v4] 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.

A working solution is to properly check the return values from recv,
handling a closed socket once there is no more data to read.

Also enable polling for G_IO_NVAL to ensure the callback is called
for all possible events as these should now be possible to handle
with the improved error detection.

Signed-off-by: Nils Carlson <pyssling@ludd.ltu.se>
---
 qemu-char.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

On 19/07/2015 22:39, pyssling@ludd.ltu.se wrote:
> 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.
> 
> A working solution is to properly check the return values from recv,
> handling a closed socket once there is no more data to read.
> 
> Also enable polling for G_IO_NVAL to ensure the callback is called
> for all possible events as these should now be possible to handle
> with the improved error detection.
> 
> Signed-off-by: Nils Carlson <pyssling@ludd.ltu.se>
> ---
>  qemu-char.c |   15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 617e034..3cbfe3e 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -807,7 +807,8 @@ static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_)
>      }
>  
>      if (now_active) {
> -        iwp->src = g_io_create_watch(iwp->channel, G_IO_IN | G_IO_ERR | G_IO_HUP);
> +        iwp->src = g_io_create_watch(iwp->channel,
> +                                     G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
>          g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
>          g_source_attach(iwp->src, NULL);
>      } else {
> @@ -2847,12 +2848,6 @@ 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) {
> -        /* connection closed */
> -        tcp_chr_disconnect(chr);
> -        return TRUE;
> -    }
> -
>      if (!s->connected || s->max_size <= 0) {
>          return TRUE;
>      }
> @@ -2860,7 +2855,11 @@ 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 &&
> +         !(errno == EAGAIN ||
> +           errno == EWOULDBLOCK ||
> +           errno == EINTR))) {

You need to check socket_error() here instead of errno.  Also, EINTR is
not a disconnection.  However, I can fix these up for you.  I'll send a
pull request.

Paolo

>          /* connection closed */
>          tcp_chr_disconnect(chr);
>      } else if (size > 0) {
>

On Tue, 21 Jul 2015, Paolo Bonzini wrote:

>
>
> On 19/07/2015 22:39, pyssling@ludd.ltu.se wrote:
>> 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.
>>
>> A working solution is to properly check the return values from recv,
>> handling a closed socket once there is no more data to read.
>>
>> Also enable polling for G_IO_NVAL to ensure the callback is called
>> for all possible events as these should now be possible to handle
>> with the improved error detection.
>>
>> Signed-off-by: Nils Carlson <pyssling@ludd.ltu.se>
>> ---
>>  qemu-char.c |   15 +++++++--------
>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 617e034..3cbfe3e 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -807,7 +807,8 @@ static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_)
>>      }
>>
>>      if (now_active) {
>> -        iwp->src = g_io_create_watch(iwp->channel, G_IO_IN | G_IO_ERR | G_IO_HUP);
>> +        iwp->src = g_io_create_watch(iwp->channel,
>> +                                     G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
>>          g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
>>          g_source_attach(iwp->src, NULL);
>>      } else {
>> @@ -2847,12 +2848,6 @@ 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) {
>> -        /* connection closed */
>> -        tcp_chr_disconnect(chr);
>> -        return TRUE;
>> -    }
>> -
>>      if (!s->connected || s->max_size <= 0) {
>>          return TRUE;
>>      }
>> @@ -2860,7 +2855,11 @@ 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 &&
>> +         !(errno == EAGAIN ||
>> +           errno == EWOULDBLOCK ||
>> +           errno == EINTR))) {
>
> You need to check socket_error() here instead of errno.  Also, EINTR is
> not a disconnection.  However, I can fix these up for you.  I'll send a
> pull request.
>

Any luck with this fixup? Let me know if you need anything more from me,
code or testing.

Thanks,
Nils


> Paolo
>
>>          /* connection closed */
>>          tcp_chr_disconnect(chr);
>>      } else if (size > 0) {
>>
>
>
>

On 23/07/2015 10:24, Nils Carlson wrote:
> Any luck with this fixup? Let me know if you need anything more from me,
> code or testing.

Just busy, I'll send a pull request today or tomorrow.

Paolo