Re: [Qemu-devel] [PATCH V2] chardev: fix parallel device can't be reconnect

[Paolo Bonzini]

On 11/07/2017 11:15, Daniel P. Berrange wrote:
> On Tue, Jul 11, 2017 at 10:30:14AM +0200, Paolo Bonzini wrote:
>> On 11/07/2017 13:47, Peng Hao wrote:
>>> Parallel device don't register be->chr_can_read function, but remote
>>> disconnect event is handled in chr_read.So connected parallel device
>>> can not detect remote disconnect event. The chardevs with chr_can_read=NULL
>>> has the same problem.
>>>
>>> Signed-off-by: Peng Hao <address@hidden>
>>> Reviewed-by: Wang Yechao <address@hidden>
>>> ---
>>>  chardev/char-socket.c  | 11 +++++++++++
>>>  chardev/char.c         | 10 ++++++++++
>>>  include/chardev/char.h |  9 +++++++++
>>>  3 files changed, 30 insertions(+)
>>>
>>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>>> index ccc499c..aa44f8f 100644
>>> --- a/chardev/char-socket.c
>>> +++ b/chardev/char-socket.c
>>> @@ -139,6 +139,9 @@ static int tcp_chr_read_poll(void *opaque)
>>>          return 0;
>>>      }
>>>      s->max_size = qemu_chr_be_can_write(chr);
>>> +    if (qemu_chr_null_be_can_read(chr)) {
>>> +        return 1;
>>> +    }
>>>      return s->max_size;
>>>  }
>>>  
>>> @@ -422,6 +425,14 @@ static gboolean tcp_chr_read(QIOChannel *chan, 
>>> GIOCondition cond, void *opaque)
>>>      uint8_t buf[CHR_READ_BUF_LEN];
>>>      int len, size;
>>>  
>>> +    if (qemu_chr_null_be_can_read(chr)) {
>>> +        size = tcp_chr_recv(chr, (void *)buf, CHR_READ_BUF_LEN);
>>
>> It would be better not to destroy data in the channel, because the
>> device could set handlers later.
>>
>> Daniel, maybe QIOChannel could have a function to check for hung-up
>> channels and return a bool?  For file descriptors it would poll the file
>> descriptor and check for POLLHUP, while other channels would have a more
>> or less obvious implementation.
> 
> IMHO the chardev code all needs to switch over to using even driven I/O
> using the  qio_channel_add_watch() function.

This is a slightly different case; qio_channel_add_watch() is for the
write side, while in this case we're concerned with the read side.

The read-side watch is managed automatically by common chardev code.
I'll see if we can change that code to set up a G_IO_HUP watch in
addition to G_IO_IN.

Paolo

>   commit 6ab3fc32ea640026726bc5f9f4db622d0954fb8a
>   Author: Daniel P. Berrange <address@hidden>
>   Date:   Tue Sep 6 14:56:04 2016 +0100
> 
>     hw: replace most use of qemu_chr_fe_write with qemu_chr_fe_write_all
> 
> Some places in the chardev code in fact alreadying use watches for a few
> actions, but then use sync i/o for other stuff. So using events will give
> a consistent model. Once you're doing proper event driven I/O, then
> detecting hangup is also trivial. 
> 
> IOW, I don't think we need add anything to QIOChannel - just continue to
> cleanup the chardev backends to better use the functionality that already
> exists.
> 
> Regards,
> Daniel
>