[V3] qemu-char: Add qemu_chr_add_handlers_full() for GMaincontext

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.fnst@cn.fujitsu.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 include/sysemu/char.h |  11 ++++-
 qemu-char.c           | 117 +++++++++++++++++++++++++++++++-------------------
 2 files changed, 83 insertions(+), 45 deletions(-)

Hi~~ Paolo and Daniel.

No news for a week~

Can you give me some feedback for this patch?

We need comments...


Thanks

Zhang Chen


On 07/28/2016 05:12 PM, 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.fnst@cn.fujitsu.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@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);
>       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)
>   {
>       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;
>

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.fnst@cn.fujitsu.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@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 <berrange@redhat.com>

It is functionally fine, regardless of the rename nitpick I mention.

Regards,
Daniel

On 08/04/2016 04:05 PM, Daniel P. Berrange wrote:
> 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.fnst@cn.fujitsu.com>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Signed-off-by: Wen Congyang <wency@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.

OK~ I will fix this in next version.

>
>>       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.

I got your point. will fix it.

>
>>   {
>>       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 <berrange@redhat.com>
>
> It is functionally fine, regardless of the rename nitpick I mention.

Thanks for your review~~
Zhang Chen

> Regards,
> Daniel