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

[Paolo Bonzini <pbonzini@redhat.com>]

On 11/07/2017 14:37, Daniel P. Berrange wrote:
> On Tue, Jul 11, 2017 at 02:21:08PM +0200, Paolo Bonzini wrote:
>> 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 <peng.hao2@zte.com.cn>
>>>>> Reviewed-by: Wang Yechao <wang.yechao255@zte.com.cn>
>>>>> ---
>>>>>  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.
> 
> NB You don't ever need to use G_IO_HUP as an explicit input flag - poll()
> will always report G_IO_HUP if you asked it to monitor for G_IO_IN.

Right, but in this case we don't want to monitor G_IO_IN, because there
is no listener.  The parallel port is a special case but actually the
same would happen for any other device whose can_read callback can
return zero.

Some Unix variants require you to specify G_IO_HUP in the events
(FreeBSD), others don't (Linux).

Paolo