[v2] chardev: fix parallel device can't be reconnect

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

Posted by Peng Hao 6 years, 9 months ago

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);
+        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) {
+        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
-- 
1.8.3.1

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

Posted by Paolo Bonzini 6 years, 9 months ago

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.

> +        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.

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] [PATCH V2] chardev: fix parallel device can't be reconnect

Posted by Daniel P. Berrange 6 years, 9 months ago

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 needed to properly
handle non-blocking I/O, to replace the workaround done in

  commit 6ab3fc32ea640026726bc5f9f4db622d0954fb8a
  Author: Daniel P. Berrange <berrange@redhat.com>
  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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

Posted by Paolo Bonzini 6 years, 9 months ago

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.

Paolo

>   commit 6ab3fc32ea640026726bc5f9f4db622d0954fb8a
>   Author: Daniel P. Berrange <berrange@redhat.com>
>   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
>

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

Posted by Marc-André Lureau 6 years, 9 months ago

Hi

On Tue, Jul 11, 2017 at 2:21 PM Paolo Bonzini <pbonzini@redhat.com> 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.
>

fwiw,  we have some code handling HUP in interesting ways, see
a6553598be42e3be899acdb153fd615cd6c3eab8
-- 
Marc-André Lureau

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

Posted by Daniel P. Berrange 6 years, 9 months ago

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.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

Posted by Paolo Bonzini 6 years, 9 months ago

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