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 <berra...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
>
> 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 | 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 :|
