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

[Zifei Tong]

Hi Markus,

On Thu, Sep 18, 2014 at 8:11 PM, Markus Armbruster <address@hidden> wrote:
> Zifei Tong <address@hidden> writes:
>
>> 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 <address@hidden>
>> Cc: Nikolay Nikolaev <address@hidden>
>> Cc: Markus Armbruster <address@hidden>
>> Cc: Anthony Liguori <address@hidden>
>> Signed-off-by: Zifei Tong <address@hidden>
>> ---
>>  qemu-char.c | 8 +-------
>>  1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 2a3cb9f..6cc69fa 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,7 +2699,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 && !(errno == EAGAIN || errno == EINTR))) {
>>          /* connection closed */
>>          tcp_chr_disconnect(chr);
>>      } else if (size > 0) {
>
> What about EWOULDBLOCK?

I thought EAGAIN and EWOULDBLOCK was the same thing, until Google told
me it's not the case on some platforms like HP-UX, AIX and Tru64.

Shall we add one more check to support these old platforms?

> The /* connection closed */ comment is now more inaccurate :)
>
> If size == 0, the peer has shut down its sending end of the connection.
>
> If size < 0, our attempt to receive encountered an error.
>
> I'd just drop the comment.

Will do this in v3.

Thanks,
Zifei