Re: [Qemu-devel] [PATCH V3] qemu-char: Add qemu_chr_add_handlers

qemu-devel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V3] qemu-char: Add qemu_chr_add_handlers_full()

From:	Zhang Chen
Subject:	Re: [Qemu-devel] [PATCH V3] qemu-char: Add qemu_chr_add_handlers_full() for GMaincontext
Date:	Thu, 4 Aug 2016 16:14:55 +0800
User-agent:	Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0



On 08/04/2016 04:05 PM, Daniel P. Berrange wrote:

On Thu, Jul 28, 2016 at 05:12:25PM +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.

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 |  11 ++++-
  qemu-char.c           | 117 +++++++++++++++++++++++++++++++-------------------
  2 files changed, 83 insertions(+), 45 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);

If I were to nitpick, I'd say we probably didn't need to rename
this struct field - just adding the extra param is enough. Only
the public API needs the _full suffix.


OK~ I will fix this in next version.

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

Likewise we could avoid the rename of this and similar internal only
methods, which would cut a few lines from the patch.


I got your point. will fix it.

  {
      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,30 +913,33 @@ 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;
  }
@@ -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;

Reviewed-by: Daniel P. Berrange <address@hidden>

It is functionally fine, regardless of the rename nitpick I mention.


Thanks for your review~~
Zhang Chen

Regards,
Daniel


--
Thanks
zhangchen

[Prev in Thread]

Current Thread

[Next in Thread]

Re: [Qemu-devel] [PATCH V3] qemu-char: Add qemu_chr_add_handlers_full() for GMaincontext, Zhang Chen, 2016/08/03
- Re: [Qemu-devel] [PATCH V3] qemu-char: Add qemu_chr_add_handlers_full() for GMaincontext, Daniel P. Berrange, 2016/08/04
  - Re: [Qemu-devel] [PATCH V3] qemu-char: Add qemu_chr_add_handlers_full() for GMaincontext, Zhang Chen <=

Prev by Date: Re: [Qemu-devel] [PATCH 3/5] sclpconsole: remove bogus check for -EAGAIN
Next by Date: Re: [Qemu-devel] [PATCH 4/5] hw: replace most use of qemu_chr_fe_write with qemu_chr_fe_write_all
Previous by thread: Re: [Qemu-devel] [PATCH V3] qemu-char: Add qemu_chr_add_handlers_full() for GMaincontext
Next by thread: [Qemu-devel] [PATCH v3 0/2] Two virtio-blk fixes
Index(es):
- Date
- Thread