Re: [Qemu-devel] [PATCH V2] chardev: fix parallel device can't be reconnect
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
> Reviewed-by: Wang Yechao
> ---
> 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
Re: [Qemu-devel] [PATCH V2] chardev: fix parallel device can't be reconnect
Hi
On Tue, Jul 11, 2017 at 2:21 PM 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
> >>> Reviewed-by: Wang Yechao
> >>> ---
> >>> 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
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
> >>> Reviewed-by: Wang Yechao
> >>> ---
> >>> 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
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
>>> Reviewed-by: Wang Yechao
>>> ---
>>> 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
> 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
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
> > Reviewed-by: Wang Yechao
> > ---
> > 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
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
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
> Reviewed-by: Wang Yechao
> ---
> 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
>
[Qemu-devel] [PATCH V2] chardev: fix parallel device can't be reconnect
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
Reviewed-by: Wang Yechao
---
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
