Re: [Qemu-devel] [RFC PATCH V9 5/7] qemu-char: Add qemu_chr_add_handlers_full() for GMaincontext

[Zhang Chen <zhangchen.fnst@cn.fujitsu.com>]

add to: Daniel P . Berrange


On 07/22/2016 02:56 PM, Zhang Chen wrote:
>
>
> On 07/22/2016 02:45 PM, Li Zhijian wrote:
>>
>>
>> On 07/22/2016 01:38 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.
>>>
>>> Cc: Daniel P . Berrange <berrange@redhat.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>
>>> 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           | 119 
>>> +++++++++++++++++++++++++++++++-------------------
>>>   2 files changed, 84 insertions(+), 46 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..0a45c9e 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,33 +913,36 @@ 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;
>>> -}
>>> + }
>> redundant modification
>
> I miss it...
> will fix in next version....
>
> Should we make this patch independent with this patch serise?
>
> Thanks
> Zhang Chen
>
>>
>> Thanks
>> Li Zhijian
>>
>>>
>>>   static void io_remove_watch_poll(guint 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;
>>>
>>>
>> .
>>
>

-- 
Thanks
zhangchen