From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51356) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVDfK-0001ME-VD for qemu-devel@nongnu.org; Thu, 04 Aug 2016 04:06:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bVDfG-0000q4-Ll for qemu-devel@nongnu.org; Thu, 04 Aug 2016 04:06:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58004) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVDfG-0000pa-E4 for qemu-devel@nongnu.org; Thu, 04 Aug 2016 04:06:02 -0400 Date: Thu, 4 Aug 2016 09:05:57 +0100 From: "Daniel P. Berrange" Message-ID: <20160804080557.GB5124@redhat.com> Reply-To: "Daniel P. Berrange" References: <1469697145-1401-1-git-send-email-zhangchen.fnst@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1469697145-1401-1-git-send-email-zhangchen.fnst@cn.fujitsu.com> Subject: Re: [Qemu-devel] [PATCH V3] qemu-char: Add qemu_chr_add_handlers_full() for GMaincontext List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Zhang Chen Cc: qemu devel , Paolo Bonzini , Jason Wang , Li Zhijian , Wen Congyang , "eddie . dong" On Thu, Jul 28, 2016 at 05:12:25PM +0800, Zhang Chen wrote: > Add qemu_chr_add_handlers_full() API, we can use > this API pass in a GMainContext,make handler run > in the context rather than main_loop. > This comments from Daniel P . Berrange. > > Signed-off-by: Zhang Chen > Signed-off-by: Li Zhijian > Signed-off-by: Wen Congyang > --- > include/sysemu/char.h | 11 ++++- > qemu-char.c | 117 +++++++++++++++++++++++++++++++------------------- > 2 files changed, 83 insertions(+), 45 deletions(-) > > diff --git a/include/sysemu/char.h b/include/sysemu/char.h > index 307fd8f..86888bc 100644 > --- a/include/sysemu/char.h > +++ b/include/sysemu/char.h > @@ -65,7 +65,8 @@ struct CharDriverState { > int (*chr_sync_read)(struct CharDriverState *s, > const uint8_t *buf, int len); > GSource *(*chr_add_watch)(struct CharDriverState *s, GIOCondition cond); > - void (*chr_update_read_handler)(struct CharDriverState *s); > + void (*chr_update_read_handler_full)(struct CharDriverState *s, > + GMainContext *context); If I were to nitpick, I'd say we probably didn't need to rename this struct field - just adding the extra param is enough. Only the public API needs the _full suffix. > int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg); > int (*get_msgfds)(struct CharDriverState *s, int* fds, int num); > int (*set_msgfds)(struct CharDriverState *s, int *fds, int num); > @@ -388,6 +389,14 @@ void qemu_chr_add_handlers(CharDriverState *s, > IOEventHandler *fd_event, > void *opaque); > > +/* This API can make handler run in the context what you pass to. */ > +void qemu_chr_add_handlers_full(CharDriverState *s, > + IOCanReadHandler *fd_can_read, > + IOReadHandler *fd_read, > + IOEventHandler *fd_event, > + void *opaque, > + GMainContext *context); > + > void qemu_chr_be_generic_open(CharDriverState *s); > void qemu_chr_accept_input(CharDriverState *s); > int qemu_chr_add_client(CharDriverState *s, int fd); > diff --git a/qemu-char.c b/qemu-char.c > index b597ee1..c544427 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -448,11 +448,12 @@ void qemu_chr_fe_printf(CharDriverState *s, const char *fmt, ...) > > static void remove_fd_in_watch(CharDriverState *chr); > > -void qemu_chr_add_handlers(CharDriverState *s, > - IOCanReadHandler *fd_can_read, > - IOReadHandler *fd_read, > - IOEventHandler *fd_event, > - void *opaque) > +void qemu_chr_add_handlers_full(CharDriverState *s, > + IOCanReadHandler *fd_can_read, > + IOReadHandler *fd_read, > + IOEventHandler *fd_event, > + void *opaque, > + GMainContext *context) > { > int fe_open; > > @@ -466,8 +467,9 @@ void qemu_chr_add_handlers(CharDriverState *s, > s->chr_read = fd_read; > s->chr_event = fd_event; > s->handler_opaque = opaque; > - if (fe_open && s->chr_update_read_handler) > - s->chr_update_read_handler(s); > + if (fe_open && s->chr_update_read_handler_full) { > + s->chr_update_read_handler_full(s, context); > + } > > if (!s->explicit_fe_open) { > qemu_chr_fe_set_open(s, fe_open); > @@ -480,6 +482,16 @@ void qemu_chr_add_handlers(CharDriverState *s, > } > } > > +void qemu_chr_add_handlers(CharDriverState *s, > + IOCanReadHandler *fd_can_read, > + IOReadHandler *fd_read, > + IOEventHandler *fd_event, > + void *opaque) > +{ > + qemu_chr_add_handlers_full(s, fd_can_read, fd_read, > + fd_event, opaque, NULL); > +} > + > static int null_chr_write(CharDriverState *chr, const uint8_t *buf, int len) > { > return len; > @@ -717,7 +729,8 @@ static void mux_chr_event(void *opaque, int event) > mux_chr_send_event(d, i, event); > } > > -static void mux_chr_update_read_handler(CharDriverState *chr) > +static void mux_chr_update_read_handler_full(CharDriverState *chr, > + GMainContext *context) Likewise we could avoid the rename of this and similar internal only methods, which would cut a few lines from the patch. > { > MuxDriver *d = chr->opaque; > > @@ -731,8 +744,10 @@ static void mux_chr_update_read_handler(CharDriverState *chr) > d->chr_event[d->mux_cnt] = chr->chr_event; > /* Fix up the real driver with mux routines */ > if (d->mux_cnt == 0) { > - qemu_chr_add_handlers(d->drv, mux_chr_can_read, mux_chr_read, > - mux_chr_event, chr); > + qemu_chr_add_handlers_full(d->drv, mux_chr_can_read, > + mux_chr_read, > + mux_chr_event, > + chr, context); > } > if (d->focus != -1) { > mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT); > @@ -813,7 +828,7 @@ static CharDriverState *qemu_chr_open_mux(const char *id, > d->drv = drv; > d->focus = -1; > chr->chr_write = mux_chr_write; > - chr->chr_update_read_handler = mux_chr_update_read_handler; > + chr->chr_update_read_handler_full = mux_chr_update_read_handler_full; > chr->chr_accept_input = mux_chr_accept_input; > /* Frontend guest-open / -close notification is not support with muxes */ > chr->chr_set_fe_open = NULL; > @@ -840,6 +855,7 @@ typedef struct IOWatchPoll > IOCanReadHandler *fd_can_read; > GSourceFunc fd_read; > void *opaque; > + GMainContext *context; > } IOWatchPoll; > > static IOWatchPoll *io_watch_poll_from_source(GSource *source) > @@ -847,7 +863,8 @@ static IOWatchPoll *io_watch_poll_from_source(GSource *source) > return container_of(source, IOWatchPoll, parent); > } > > -static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_) > +static gboolean io_watch_poll_prepare_full(GSource *source, > + gint *timeout_) > { > IOWatchPoll *iwp = io_watch_poll_from_source(source); > bool now_active = iwp->fd_can_read(iwp->opaque) > 0; > @@ -860,7 +877,7 @@ static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_) > iwp->src = qio_channel_create_watch( > iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL); > g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL); > - g_source_attach(iwp->src, NULL); > + g_source_attach(iwp->src, iwp->context); > } else { > g_source_destroy(iwp->src); > g_source_unref(iwp->src); > @@ -896,30 +913,33 @@ static void io_watch_poll_finalize(GSource *source) > assert(iwp->src == NULL); > } > > -static GSourceFuncs io_watch_poll_funcs = { > - .prepare = io_watch_poll_prepare, > +static GSourceFuncs io_watch_poll_funcs_full = { > + .prepare = io_watch_poll_prepare_full, > .check = io_watch_poll_check, > .dispatch = io_watch_poll_dispatch, > .finalize = io_watch_poll_finalize, > }; > > /* Can only be used for read */ > -static guint io_add_watch_poll(QIOChannel *ioc, > - IOCanReadHandler *fd_can_read, > - QIOChannelFunc fd_read, > - gpointer user_data) > +static guint io_add_watch_poll_full(QIOChannel *ioc, > + IOCanReadHandler *fd_can_read, > + QIOChannelFunc fd_read, > + gpointer user_data, > + GMainContext *context) > { > IOWatchPoll *iwp; > int tag; > > - iwp = (IOWatchPoll *) g_source_new(&io_watch_poll_funcs, sizeof(IOWatchPoll)); > + iwp = (IOWatchPoll *) g_source_new(&io_watch_poll_funcs_full, > + sizeof(IOWatchPoll)); > iwp->fd_can_read = fd_can_read; > iwp->opaque = user_data; > iwp->ioc = ioc; > iwp->fd_read = (GSourceFunc) fd_read; > iwp->src = NULL; > + iwp->context = context; > > - tag = g_source_attach(&iwp->parent, NULL); > + tag = g_source_attach(&iwp->parent, context); > g_source_unref(&iwp->parent); > return tag; > } > @@ -1051,15 +1071,17 @@ static GSource *fd_chr_add_watch(CharDriverState *chr, GIOCondition cond) > return qio_channel_create_watch(s->ioc_out, cond); > } > > -static void fd_chr_update_read_handler(CharDriverState *chr) > +static void fd_chr_update_read_handler_full(CharDriverState *chr, > + GMainContext *context) > { > FDCharDriver *s = chr->opaque; > > remove_fd_in_watch(chr); > if (s->ioc_in) { > - chr->fd_in_tag = io_add_watch_poll(s->ioc_in, > - fd_chr_read_poll, > - fd_chr_read, chr); > + chr->fd_in_tag = io_add_watch_poll_full(s->ioc_in, > + fd_chr_read_poll, > + fd_chr_read, chr, > + context); > } > } > > @@ -1098,7 +1120,7 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out, > chr->opaque = s; > chr->chr_add_watch = fd_chr_add_watch; > chr->chr_write = fd_chr_write; > - chr->chr_update_read_handler = fd_chr_update_read_handler; > + chr->chr_update_read_handler_full = fd_chr_update_read_handler_full; > chr->chr_close = fd_chr_close; > > return chr; > @@ -1303,7 +1325,8 @@ static void pty_chr_update_read_handler_locked(CharDriverState *chr) > } > } > > -static void pty_chr_update_read_handler(CharDriverState *chr) > +static void pty_chr_update_read_handler_full(CharDriverState *chr, > + GMainContext *context) > { > qemu_mutex_lock(&chr->chr_write_lock); > pty_chr_update_read_handler_locked(chr); > @@ -1405,9 +1428,10 @@ static void pty_chr_state(CharDriverState *chr, int connected) > s->open_tag = g_idle_add(qemu_chr_be_generic_open_func, chr); > } > if (!chr->fd_in_tag) { > - chr->fd_in_tag = io_add_watch_poll(s->ioc, > - pty_chr_read_poll, > - pty_chr_read, chr); > + chr->fd_in_tag = io_add_watch_poll_full(s->ioc, > + pty_chr_read_poll, > + pty_chr_read, > + chr, NULL); > } > } > } > @@ -1464,7 +1488,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id, > s = g_new0(PtyCharDriver, 1); > chr->opaque = s; > chr->chr_write = pty_chr_write; > - chr->chr_update_read_handler = pty_chr_update_read_handler; > + chr->chr_update_read_handler_full = pty_chr_update_read_handler_full; > chr->chr_close = pty_chr_close; > chr->chr_add_watch = pty_chr_add_watch; > chr->explicit_be_open = true; > @@ -2546,15 +2570,17 @@ static gboolean udp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque) > return TRUE; > } > > -static void udp_chr_update_read_handler(CharDriverState *chr) > +static void udp_chr_update_read_handler_full(CharDriverState *chr, > + GMainContext *context) > { > NetCharDriver *s = chr->opaque; > > remove_fd_in_watch(chr); > if (s->ioc) { > - chr->fd_in_tag = io_add_watch_poll(s->ioc, > - udp_chr_read_poll, > - udp_chr_read, chr); > + chr->fd_in_tag = io_add_watch_poll_full(s->ioc, > + udp_chr_read_poll, > + udp_chr_read, chr, > + context); > } > } > > @@ -2588,7 +2614,7 @@ static CharDriverState *qemu_chr_open_udp(QIOChannelSocket *sioc, > s->bufptr = 0; > chr->opaque = s; > chr->chr_write = udp_chr_write; > - chr->chr_update_read_handler = udp_chr_update_read_handler; > + chr->chr_update_read_handler_full = udp_chr_update_read_handler_full; > chr->chr_close = udp_chr_close; > /* be isn't opened until we get a connection */ > chr->explicit_be_open = true; > @@ -2929,14 +2955,16 @@ static void tcp_chr_connect(void *opaque) > > s->connected = 1; > if (s->ioc) { > - chr->fd_in_tag = io_add_watch_poll(s->ioc, > - tcp_chr_read_poll, > - tcp_chr_read, chr); > + chr->fd_in_tag = io_add_watch_poll_full(s->ioc, > + tcp_chr_read_poll, > + tcp_chr_read, > + chr, NULL); > } > qemu_chr_be_generic_open(chr); > } > > -static void tcp_chr_update_read_handler(CharDriverState *chr) > +static void tcp_chr_update_read_handler_full(CharDriverState *chr, > + GMainContext *context) > { > TCPCharDriver *s = chr->opaque; > > @@ -2946,9 +2974,10 @@ static void tcp_chr_update_read_handler(CharDriverState *chr) > > remove_fd_in_watch(chr); > if (s->ioc) { > - chr->fd_in_tag = io_add_watch_poll(s->ioc, > - tcp_chr_read_poll, > - tcp_chr_read, chr); > + chr->fd_in_tag = io_add_watch_poll_full(s->ioc, > + tcp_chr_read_poll, > + tcp_chr_read, chr, > + context); > } > } > > @@ -4409,7 +4438,7 @@ static CharDriverState *qmp_chardev_open_socket(const char *id, > chr->set_msgfds = tcp_set_msgfds; > chr->chr_add_client = tcp_chr_add_client; > chr->chr_add_watch = tcp_chr_add_watch; > - chr->chr_update_read_handler = tcp_chr_update_read_handler; > + chr->chr_update_read_handler_full = tcp_chr_update_read_handler_full; > /* be isn't opened until we get a connection */ > chr->explicit_be_open = true; Reviewed-by: Daniel P. Berrange It is functionally fine, regardless of the rename nitpick I mention. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|