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 <zhangchen.f...@cn.fujitsu.com>
> Signed-off-by: Li Zhijian <lizhij...@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <we...@cn.fujitsu.com>
> ---
> 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 <berra...@redhat.com>
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 :|
