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

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

[peng.hao2]

发件人： <pbonzini@redhat.com>
收件人：彭浩10096742 <marcandre.lureau@redhat.com>
抄送人： <qemu-devel@nongnu.org>王业超10154425 <berrange@redhat.com>
日 期 ：2017年07月11日 16:31
主 题 ：Re: [PATCH V2] chardev: fix parallel device can't be reconnect





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.
------This device do not register a chr_read function,so it's safe to destroy data(no read handler).




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.

> +        if (size == 0 || size == -1) {
> +            tcp_chr_disconnect(chr)
> +        }
> +        return TRUE
> +    }
> +
>      if (!s->connected || s->max_size <= 0) {
>          return TRUE
>      }
> diff --git a/chardev/char.c b/chardev/char.c
> index 2b679a2..7f7f486 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -148,6 +148,16 @@ int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all)
>      return offset
>  }
>  
> +int qemu_chr_null_be_can_read(Chardev *s)
> +{
> +    CharBackend *be = s->be
> +
> +    if (!be || !be->chr_can_read) {

I think it's really chr_read that you want to consider here.

----if chr_can_read is null,then chr_read is null too.


Thanks,

Paolo

> +        return 1
> +    }
> +    return 0
> +}
> +
>  int qemu_chr_be_can_write(Chardev *s)
>  {
>      CharBackend *be = s->be
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 8a9ade4..8dd28a8 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -114,6 +114,15 @@ void qemu_chr_cleanup(void)
>  Chardev *qemu_chr_new_noreplay(const char *label, const char *filename)
>  
>  /**
> + * @qemu_chr_null_be_can_read:
> + *
> + * Check if Chardev's chr_can_read is registered.
> + *
> + * Returns: 1 if Chardev's chr_can_read is null.
> + */
> +int qemu_chr_null_be_can_read(Chardev *s)
> +
> +/**
>   * @qemu_chr_be_can_write:
>   *
>   * Determine how much data the front end can currently accept.  This function
>

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

[Paolo Bonzini]

On 11/07/2017 11:33, peng.hao2@zte.com.cn wrote:
>>> +    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.
>
> This device do not register a chr_read function,so it's safe to
> destroy data(no read handler).

I agree, but this code doesn't know that the device is not changing
handlers later.

Paolo