[Qemu-devel] [PATCH 1/2] iohandler: Convert I/O handler to GSource

[Fam Zheng]

Previously, I/O handler fd's are hooked into main loop by:

 1) qemu_iohandler_fill to add the list of io handler fds to g_poll.

 2) qemu_iohandler_poll to check the revent values and do the dispatch.

This patch moves all the fds into a GSource, which is attached to the
main event loop. This way we don't have to rebuild the whole list of fds
for every iteration, and there is a cleaner interface between us and
main loop. Furthermore, it makes adding a Linux specific implementation
(epoll) a lot easier.

Signed-off-by: Fam Zheng <address@hidden>
---
 Makefile.objs            |   2 +-
 include/qemu/iohandler.h |  52 ++++++++++++++++
 include/qemu/main-loop.h |   2 -
 iohandler-posix.c        | 150 +++++++++++++++++++++++++++++++++++++++++++++++
 iohandler.c              |  90 +++-------------------------
 main-loop.c              |   6 +-
 6 files changed, 215 insertions(+), 87 deletions(-)
 create mode 100644 include/qemu/iohandler.h
 create mode 100644 iohandler-posix.c

diff --git a/Makefile.objs b/Makefile.objs
index 97db978..55dbc36 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -8,7 +8,7 @@ util-obj-y = util/ qobject/ qapi/ qapi-types.o qapi-visit.o 
qapi-event.o
 
 block-obj-y = async.o thread-pool.o
 block-obj-y += nbd.o block.o blockjob.o
-block-obj-y += main-loop.o iohandler.o qemu-timer.o
+block-obj-y += main-loop.o iohandler.o qemu-timer.o iohandler-posix.o
 block-obj-$(CONFIG_POSIX) += aio-posix.o
 block-obj-$(CONFIG_WIN32) += aio-win32.o
 block-obj-y += block/
diff --git a/include/qemu/iohandler.h b/include/qemu/iohandler.h
new file mode 100644
index 0000000..e2af47d
--- /dev/null
+++ b/include/qemu/iohandler.h
@@ -0,0 +1,52 @@
+/*
+ * QEMU I/O Handler
+ *
+ * Copyright (c) 2014 Red Hat, Inc.
+ *
+ * Author: Fam Zheng <address@hidden>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef QEMU_IOHANDLER_H
+#define QEMU_IOHANDLER_H
+
+#include "qemu/main-loop.h"
+
+typedef struct IOHandlerRecord {
+    IOCanReadHandler *fd_read_poll;
+    IOHandler *fd_read;
+    IOHandler *fd_write;
+    void *opaque;
+    QLIST_ENTRY(IOHandlerRecord) next;
+    int fd;
+    bool deleted;
+    GPollFD gpfd;
+    bool attached;
+} IOHandlerRecord;
+
+typedef struct {
+    GSource source;
+
+    QLIST_HEAD(, IOHandlerRecord) io_handlers;
+} IOHandlerSource;
+
+GSource *qemu_iohandler_get_source(void);
+
+#endif
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index 62c68c0..065944c 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -302,8 +302,6 @@ void qemu_mutex_unlock_iothread(void);
 /* internal interfaces */
 
 void qemu_fd_register(int fd);
-void qemu_iohandler_fill(GArray *pollfds);
-void qemu_iohandler_poll(GArray *pollfds, int rc);
 
 QEMUBH *qemu_bh_new(QEMUBHFunc *cb, void *opaque);
 void qemu_bh_schedule_idle(QEMUBH *bh);
diff --git a/iohandler-posix.c b/iohandler-posix.c
new file mode 100644
index 0000000..0fac91f
--- /dev/null
+++ b/iohandler-posix.c
@@ -0,0 +1,150 @@
+/*
+ * I/O Handler posix implementation
+ *
+ * Copyright (c) 2014 Red Hat, Inc.
+ *
+ * Author: Fam Zheng <address@hidden>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "config-host.h"
+#include "qemu-common.h"
+#include "qemu/iohandler.h"
+
+/* Prepare for the poll by synchronize IO handler list to GSource. */
+static gboolean iohandler_source_prepare(GSource *source, gint *timeout)
+{
+    IOHandlerRecord *ioh;
+    IOHandlerSource *s = (IOHandlerSource *)source;
+
+    QLIST_FOREACH(ioh, &s->io_handlers, next) {
+        bool add = false, remove = false;
+
+        if (ioh->deleted) {
+            remove = ioh->attached;
+        } else {
+            int events = 0;
+            if (ioh->fd_read &&
+                (!ioh->fd_read_poll ||
+                 ioh->fd_read_poll(ioh->opaque) != 0)) {
+                events |= G_IO_IN | G_IO_HUP | G_IO_ERR;
+            }
+            if (ioh->fd_write) {
+                events |= G_IO_OUT | G_IO_ERR;
+            }
+            ioh->gpfd.events = events,
+
+            remove = !events;
+            add = events && !ioh->attached;
+        }
+        if (remove) {
+            assert(!add);
+            g_source_remove_poll(source, &ioh->gpfd);
+            ioh->attached = false;
+        }
+        if (add) {
+            g_source_add_poll(source, &ioh->gpfd);
+            ioh->attached = true;
+        }
+    }
+    *timeout = -1;
+    return false;
+}
+
+static gboolean iohandler_source_check(GSource *source)
+{
+    IOHandlerRecord *ioh;
+    IOHandlerSource *s = (IOHandlerSource *)source;
+
+    QLIST_FOREACH(ioh, &s->io_handlers, next) {
+        int events;
+        if (!ioh->attached) {
+            continue;
+        }
+        events = ioh->gpfd.revents & ioh->gpfd.events;
+        if (ioh->fd_read &&
+                (events & (G_IO_IN | G_IO_HUP | G_IO_ERR)) &&
+                (!ioh->fd_read_poll || ioh->fd_read_poll(ioh->opaque) != 0)) {
+            return true;
+        }
+        if (ioh->fd_write && (events & (G_IO_OUT | G_IO_ERR))) {
+            return true;
+        }
+    }
+    return false;
+}
+
+static gboolean iohandler_source_dispatch(GSource *source,
+                                          GSourceFunc callback,
+                                          gpointer data)
+{
+    IOHandlerRecord *pioh, *ioh;
+    IOHandlerSource *s = (IOHandlerSource *)source;
+    int ret = false;
+
+    assert(callback == NULL);
+
+    QLIST_FOREACH_SAFE(ioh, &s->io_handlers, next, pioh) {
+        int revents = 0;
+
+        if (ioh->deleted) {
+            assert(!ioh->attached);
+        } else {
+            revents = ioh->gpfd.events & ioh->gpfd.revents;
+        }
+        if (ioh->fd_read && (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
+            ioh->fd_read(ioh->opaque);
+            ret = true;
+        }
+        if (ioh->fd_write && (revents & (G_IO_OUT | G_IO_ERR))) {
+            ioh->fd_write(ioh->opaque);
+            ret = true;
+        }
+
+        /* Do this last in case read/write handlers marked it for deletion */
+        if (ioh->deleted) {
+            if (ioh->attached) {
+                g_source_remove_poll(source, &ioh->gpfd);
+            }
+            QLIST_REMOVE(ioh, next);
+            g_free(ioh);
+        }
+    }
+    return ret;
+}
+
+static GSourceFuncs iohandler_source_funcs = {
+    iohandler_source_prepare,
+    iohandler_source_check,
+    iohandler_source_dispatch,
+    /* finalize */ NULL
+};
+
+GSource *qemu_iohandler_get_source(void)
+{
+    static IOHandlerSource *ioh_source;
+    if (!ioh_source) {
+        GSource *source = g_source_new(&iohandler_source_funcs,
+                                       sizeof(IOHandlerSource));
+        ioh_source = (IOHandlerSource *)source;
+        QLIST_INIT(&ioh_source->io_handlers);
+    }
+    return &ioh_source->source;
+}
diff --git a/iohandler.c b/iohandler.c
index cca614f..79982ee 100644
--- a/iohandler.c
+++ b/iohandler.c
@@ -27,26 +27,12 @@
 #include "qemu/queue.h"
 #include "block/aio.h"
 #include "qemu/main-loop.h"
+#include "qemu/iohandler.h"
 
 #ifndef _WIN32
 #include <sys/wait.h>
 #endif
 
-typedef struct IOHandlerRecord {
-    IOCanReadHandler *fd_read_poll;
-    IOHandler *fd_read;
-    IOHandler *fd_write;
-    void *opaque;
-    QLIST_ENTRY(IOHandlerRecord) next;
-    int fd;
-    int pollfds_idx;
-    bool deleted;
-} IOHandlerRecord;
-
-static QLIST_HEAD(, IOHandlerRecord) io_handlers =
-    QLIST_HEAD_INITIALIZER(io_handlers);
-
-
 /* XXX: fd_read_poll should be suppressed, but an API change is
    necessary in the character devices to suppress fd_can_read(). */
 int qemu_set_fd_handler2(int fd,
@@ -56,31 +42,33 @@ int qemu_set_fd_handler2(int fd,
                          void *opaque)
 {
     IOHandlerRecord *ioh;
+    IOHandlerSource *source = (IOHandlerSource *)qemu_iohandler_get_source();
 
     assert(fd >= 0);
 
     if (!fd_read && !fd_write) {
-        QLIST_FOREACH(ioh, &io_handlers, next) {
+        QLIST_FOREACH(ioh, &source->io_handlers, next) {
             if (ioh->fd == fd) {
                 ioh->deleted = 1;
                 break;
             }
         }
     } else {
-        QLIST_FOREACH(ioh, &io_handlers, next) {
+        QLIST_FOREACH(ioh, &source->io_handlers, next) {
             if (ioh->fd == fd)
                 goto found;
         }
         ioh = g_malloc0(sizeof(IOHandlerRecord));
-        QLIST_INSERT_HEAD(&io_handlers, ioh, next);
+        QLIST_INSERT_HEAD(&source->io_handlers, ioh, next);
     found:
         ioh->fd = fd;
         ioh->fd_read_poll = fd_read_poll;
         ioh->fd_read = fd_read;
         ioh->fd_write = fd_write;
         ioh->opaque = opaque;
-        ioh->pollfds_idx = -1;
-        ioh->deleted = 0;
+        ioh->deleted = false;
+        ioh->attached = false;
+        ioh->gpfd.fd = fd;
         qemu_notify_event();
     }
     return 0;
@@ -94,68 +82,6 @@ int qemu_set_fd_handler(int fd,
     return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
 }
 
-void qemu_iohandler_fill(GArray *pollfds)
-{
-    IOHandlerRecord *ioh;
-
-    QLIST_FOREACH(ioh, &io_handlers, next) {
-        int events = 0;
-
-        if (ioh->deleted)
-            continue;
-        if (ioh->fd_read &&
-            (!ioh->fd_read_poll ||
-             ioh->fd_read_poll(ioh->opaque) != 0)) {
-            events |= G_IO_IN | G_IO_HUP | G_IO_ERR;
-        }
-        if (ioh->fd_write) {
-            events |= G_IO_OUT | G_IO_ERR;
-        }
-        if (events) {
-            GPollFD pfd = {
-                .fd = ioh->fd,
-                .events = events,
-            };
-            ioh->pollfds_idx = pollfds->len;
-            g_array_append_val(pollfds, pfd);
-        } else {
-            ioh->pollfds_idx = -1;
-        }
-    }
-}
-
-void qemu_iohandler_poll(GArray *pollfds, int ret)
-{
-    if (ret > 0) {
-        IOHandlerRecord *pioh, *ioh;
-
-        QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
-            int revents = 0;
-
-            if (!ioh->deleted && ioh->pollfds_idx != -1) {
-                GPollFD *pfd = &g_array_index(pollfds, GPollFD,
-                                              ioh->pollfds_idx);
-                revents = pfd->revents;
-            }
-
-            if (!ioh->deleted && ioh->fd_read &&
-                (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
-                ioh->fd_read(ioh->opaque);
-            }
-            if (!ioh->deleted && ioh->fd_write &&
-                (revents & (G_IO_OUT | G_IO_ERR))) {
-                ioh->fd_write(ioh->opaque);
-            }
-
-            /* Do this last in case read/write handlers marked it for deletion 
*/
-            if (ioh->deleted) {
-                QLIST_REMOVE(ioh, next);
-                g_free(ioh);
-            }
-        }
-    }
-}
-
 /* reaping of zombies.  right now we're not passing the status to
    anyone, but it would be possible to add a callback.  */
 #ifndef _WIN32
diff --git a/main-loop.c b/main-loop.c
index 53393a4..4f81e9f 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -28,6 +28,7 @@
 #include "sysemu/qtest.h"
 #include "slirp/libslirp.h"
 #include "qemu/main-loop.h"
+#include "qemu/iohandler.h"
 #include "block/aio.h"
 
 #ifndef _WIN32
@@ -148,6 +149,9 @@ int qemu_init_main_loop(Error **errp)
     src = aio_get_g_source(qemu_aio_context);
     g_source_attach(src, NULL);
     g_source_unref(src);
+    src = qemu_iohandler_get_source();
+    g_source_attach(src, NULL);
+    g_source_unref(src);
     return 0;
 }
 
@@ -474,7 +478,6 @@ int main_loop_wait(int nonblocking)
 #ifdef CONFIG_SLIRP
     slirp_pollfds_fill(gpollfds, &timeout);
 #endif
-    qemu_iohandler_fill(gpollfds);
 
     if (timeout == UINT32_MAX) {
         timeout_ns = -1;
@@ -487,7 +490,6 @@ int main_loop_wait(int nonblocking)
                                           &main_loop_tlg));
 
     ret = os_host_main_loop_wait(timeout_ns);
-    qemu_iohandler_poll(gpollfds, ret);
 #ifdef CONFIG_SLIRP
     slirp_pollfds_poll(gpollfds, (ret < 0));
 #endif
-- 
1.9.3