[RFC,V8,5/7] qemu-char: Add qemu_chr_add_handlers_full() for GMaincontext

Commit Message

Zhang Chen July 21, 2016, 8:26 a.m. UTC

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 <[e-mail]>
Cc: Paolo Bonzini <[e-mail]>

Signed-off-by: Zhang Chen <[e-mail]>
Signed-off-by: Li Zhijian <[e-mail]>
Signed-off-by: Wen Congyang <[e-mail]>
---
 include/sysemu/char.h |  10 ++++
 qemu-char.c           | 145 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 155 insertions(+)

Comments

Daniel P. Berrangé July 21, 2016, 9 a.m. UTC | #1

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 <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> 
> Signed-off-by: Zhang Chen <[e-mail]>
> Signed-off-by: Li Zhijian <[e-mail]>
> Signed-off-by: Wen Congyang <[e-mail]>
> ---
>  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

Zhang Chen July 21, 2016, 9:29 a.m. UTC | #2

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 <[e-mail]>
>> Cc: Paolo Bonzini <[e-mail]>
>>
>> Signed-off-by: Zhang Chen <[e-mail]>
>> Signed-off-by: Li Zhijian <[e-mail]>
>> Signed-off-by: Wen Congyang <[e-mail]>
>> ---
>>   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

Patch

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);
+    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) {
+        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);
+    }
+}
+
 static int null_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     return len;
@@ -840,6 +874,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)
@@ -869,6 +904,29 @@  static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_)
     return FALSE;
 }
 
+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;
+    bool was_active = iwp->src != NULL;
+    if (was_active == now_active) {
+        return FALSE;
+    }
+
+    if (now_active) {
+        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, iwp->context);
+    } else {
+        g_source_destroy(iwp->src);
+        g_source_unref(iwp->src);
+        iwp->src = NULL;
+    }
+    return FALSE;
+}
+
 static gboolean io_watch_poll_check(GSource *source)
 {
     return FALSE;
@@ -903,6 +961,13 @@  static GSourceFuncs io_watch_poll_funcs = {
     .finalize = io_watch_poll_finalize,
 };
 
+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,
@@ -924,6 +989,30 @@  static guint io_add_watch_poll(QIOChannel *ioc,
     return tag;
 }
 
+/* Can only be used for read */
+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_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, context);
+    g_source_unref(&iwp->parent);
+    return tag;
+ }
+
 static void io_remove_watch_poll(guint tag)
 {
     GSource *source;
@@ -1063,6 +1152,20 @@  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_full(s->ioc_in,
+                                                fd_chr_read_poll,
+                                                fd_chr_read, chr,
+                                                context);
+    }
+}
+
 static void fd_chr_close(struct CharDriverState *chr)
 {
     FDCharDriver *s = chr->opaque;
@@ -1099,6 +1202,7 @@  static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out,
     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;
@@ -1310,6 +1414,12 @@  static void pty_chr_update_read_handler(CharDriverState *chr)
     qemu_mutex_unlock(&chr->chr_write_lock);
 }
 
+static void pty_chr_update_read_handler_full(CharDriverState *chr,
+                                             GMainContext *context)
+{
+    pty_chr_update_read_handler(chr);
+}
+
 /* Called with chr_write_lock held.  */
 static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
@@ -1465,6 +1575,7 @@  static CharDriverState *qemu_chr_open_pty(const char *id,
     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;
@@ -2558,6 +2669,20 @@  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_full(s->ioc,
+                                                udp_chr_read_poll,
+                                                udp_chr_read, chr,
+                                                context);
+    }
+}
+
 static void udp_chr_close(CharDriverState *chr)
 {
     NetCharDriver *s = chr->opaque;
@@ -2589,6 +2714,7 @@  static CharDriverState *qemu_chr_open_udp(QIOChannelSocket *sioc,
     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;
@@ -2952,6 +3078,24 @@  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;
+
+    if (!s->connected) {
+        return;
+    }
+
+    remove_fd_in_watch(chr);
+    if (s->ioc) {
+        chr->fd_in_tag = io_add_watch_poll_full(s->ioc,
+                                                tcp_chr_read_poll,
+                                                tcp_chr_read, chr,
+                                                context);
+    }
+}
+
 typedef struct {
     CharDriverState *chr;
     char buf[12];
@@ -4410,6 +4554,7 @@  static CharDriverState *qmp_chardev_open_socket(const char *id,
     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;