From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39579) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFjyq-0007ym-Pl for qemu-devel@nongnu.org; Wed, 22 Jun 2016 11:22:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFjyg-0003Kb-J5 for qemu-devel@nongnu.org; Wed, 22 Jun 2016 11:22:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44485) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFjyg-0003KX-B5 for qemu-devel@nongnu.org; Wed, 22 Jun 2016 11:22:06 -0400 Date: Wed, 22 Jun 2016 16:22:02 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20160622152202.GF6604@work-vm> References: <1466432945-28682-1-git-send-email-pbonzini@redhat.com> <1466432945-28682-6-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1466432945-28682-6-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH 5/6] char: change qemu_chr_fe_add_watch to return unsigned List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, bcketchum@gmail.com * Paolo Bonzini (pbonzini@redhat.com) wrote: > g_source_attach can return any value between 1 and UINT_MAX if you let > QEMU run long enough. However, qemu_chr_fe_add_watch can also return > a negative errno value when the device is disconnected or does not > support chr_add_watch. Change it to return zero to avoid overloading > these values. > > Fix the cadence_uart which asserts in this case (easily obtained with > "-serial pty"). > > Signed-off-by: Paolo Bonzini > --- > hw/char/cadence_uart.c | 5 ++++- > include/sysemu/char.h | 16 ++++++++++++++-- > qemu-char.c | 8 ++++---- > 3 files changed, 22 insertions(+), 7 deletions(-) > > diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c > index c856fc3..488a570 100644 > --- a/hw/char/cadence_uart.c > +++ b/hw/char/cadence_uart.c > @@ -294,7 +294,10 @@ static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond, > if (s->tx_count) { > int r = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, > cadence_uart_xmit, s); > - assert(r); > + if (!r) { > + s->tx_count = 0; > + return FALSE; > + } > } > > uart_update_status(s); > diff --git a/include/sysemu/char.h b/include/sysemu/char.h > index 1eb2d0f..07434a0 100644 > --- a/include/sysemu/char.h > +++ b/include/sysemu/char.h > @@ -221,8 +221,20 @@ void qemu_chr_fe_event(CharDriverState *s, int event); > void qemu_chr_fe_printf(CharDriverState *s, const char *fmt, ...) > GCC_FMT_ATTR(2, 3); > > -int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond, > - GIOFunc func, void *user_data); > +/** > + * @qemu_chr_fe_add_watch: > + * > + * If the backend is connected, create and add a #GSource that fires > + * when the given condition (typically G_IO_OUT|G_IO_HUP or G_IO_HUP) > + * is active; return the #GSource's tag. If it is disconnected, > + * return 0. > + * > + * @cond the condition to poll for > + * @func the function to call when the condition happens > + * @user_data the opaque pointer to pass to @func > + */ > +unsigned qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond, > + GIOFunc func, void *user_data); can we make this a guint? hw/char/virtio-console.c and monitor.c already use guint as the type for a watch and that matches the glib definition? (Although not net/vhost-user.c which still has int, but that should probably be fixed anyway). But other than that; Reviewed-by: Dr. David Alan Gilbert Dave > > /** > * @qemu_chr_fe_write: > diff --git a/qemu-char.c b/qemu-char.c > index 84f49ac..39b2ccd 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -3966,19 +3966,19 @@ void qemu_chr_fe_event(struct CharDriverState *chr, int event) > } > } > > -int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond, > - GIOFunc func, void *user_data) > +unsigned qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond, > + GIOFunc func, void *user_data) > { > GSource *src; > guint tag; > > if (s->chr_add_watch == NULL) { > - return -ENOSYS; > + return 0; > } > > src = s->chr_add_watch(s, cond); > if (!src) { > - return -EINVAL; > + return 0; > } > > g_source_set_callback(src, (GSourceFunc)func, user_data, NULL); > -- > 2.5.5 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK