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

[Zhang Chen]

On 07/21/2016 05:00 PM, Daniel P. Berrange wrote:
> On Thu, Jul 21, 2016 at 04:26:11PM +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.
> >
> > Cc: Daniel P . Berrange <address@hidden>
> > Cc: Paolo Bonzini <address@hidden>
> >
> > Signed-off-by: Zhang Chen <address@hidden>
> > Signed-off-by: Li Zhijian <address@hidden>
> > Signed-off-by: Wen Congyang <address@hidden>
> > ---
> >   include/sysemu/char.h |  10 ++++
> >   qemu-char.c           | 145 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 155 insertions(+)
> >
> > diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> > index 307fd8f..3ab897e 100644
> > --- a/include/sysemu/char.h
> > +++ b/include/sysemu/char.h
> > @@ -66,6 +66,8 @@ struct CharDriverState {
> >                            const uint8_t *buf, int len);
> >       GSource *(*chr_add_watch)(struct CharDriverState *s, GIOCondition cond);
> >       void (*chr_update_read_handler)(struct CharDriverState *s);
> I'd expect this to be deleted, and all existing impls given a
> new GMainContext * argument.

OK~~ I will fix it in next version.

> > +    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 +390,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..607b3b8 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -480,6 +480,40 @@ void qemu_chr_add_handlers(CharDriverState *s,
> >       }
> >   }
> >   
> > +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;
> > +
> > +    if (!opaque && !fd_can_read && !fd_read && !fd_event) {
> > +        fe_open = 0;
> > +        remove_fd_in_watch(s);
> > +    } else {
> > +        fe_open = 1;
> > +    }
> > +    s->chr_can_read = fd_can_read;
> > +    s->chr_read = fd_read;
> > +    s->chr_event = fd_event;
> > +    s->handler_opaque = opaque;
> > +    if (fe_open && s->chr_update_read_handler) {
> You should be checking s->chr_update_read_handler_full

Good Catch...
will fix in next version.


> > +        s->chr_update_read_handler_full(s, context);
> > +    }
> > +
> > +    if (!s->explicit_fe_open) {
> > +        qemu_chr_fe_set_open(s, fe_open);
> > +    }
> > +
> > +    /* We're connecting to an already opened device, so let's make sure we
> > +       also get the open event */
> > +    if (fe_open && s->be_open) {
> > +        qemu_chr_be_generic_open(s);
> > +    }
> > +}
> I would expect the entire of the qemu_chr_add_handlers() method body
> to be deleted and have it simply call qemu_chr_add_handlers_full(),
> passing in a NULL GMainContext. That way we avoid code duplication.

Make sense.
I got it~

Thanks a lot~~
Zhang Chen

> Regards,
> Daniel

--
Thanks
zhangchen