| Message ID | 1499773645-120137-1-git-send-email-peng.hao2@zte.com.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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 >
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
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
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
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