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

["Daniel P. Berrange" <berrange@redhat.com>]

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

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

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

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