Re: [Qemu-devel] [PATCH v3] qemu-char: Do not disconnect when there's data for reading

[Kirill Batuzov <batuzovk@ispras.ru>]

On 19.09.2014 11:12, Zifei Tong wrote:
> After commit 812c1057f6175ac9a9829fa2920a2b5783814193 (Handle G_IO_HUP
> in tcp_chr_read for tcp chardev), connections are disconnected when in
> G_IO_HUP condition.
>
> However, it's possible that there is still data for reading in the channel.
> In that case, the remaining data is not handled.
>
> I saw a related bug when running socat in write-only mode, after
>
>    $ echo "quit" | socat -u - UNIX-CONNECT:qemu-monitor
>
> the monitor won't not run the 'quit' command.
>
> Instead of GIOCondition, this patch uses the return value of tcp_chr_recv()
> to check the state of connection as suggested by Kirill.
>
> Cc: Kirill Batuzov <batuzovk@ispras.ru>
> Cc: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Anthony Liguori <aliguori@amazon.com>
> Signed-off-by: Zifei Tong <zifeitong@gmail.com>
> ---
> Changes in v3: handle EWOULDBLOCK, remove inaccurate comment
>
>   qemu-char.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 2a3cb9f..d1893a0 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2692,12 +2692,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;
>       }
> @@ -2705,8 +2699,8 @@ 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) {
> -        /* connection closed */
> +    if (size == 0 ||
> +        (size < 0 && !(errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR))) {
>           tcp_chr_disconnect(chr);
>       } else if (size > 0) {
>           if (s->do_telnetopt)
>

Looks good to me.

Reviewed-by: Kirill Batuzov <batuzovk@ispras.ru>

-- 
Kirill