[Qemu-devel] [PATCH 0/7] New iohandler API

[Qemu-devel] [PATCH 0/7] New iohandler API

[Amit Shah]

Hello,

This is a new iohandler API implementation.  It gets rid of the
multiple callbacks and switches to just one callback with an event
mask that triggered the callback.

qemu-char.c has been trivially updated to replace all usage of
qemu_set_fd_handler* functions to the new api.

Future work includes removing the qemu_set_fd_handler* functions and
getting rid of #include "qemu-char.h" from all the src files that
don't need it (most of them).

The users of the char api can then be also converted over to a new api
in similar fashion, deprecating the usage of can_read callbacks
altogether.

I tested this series in two scenarios: monitor on stdio and by running
the virtio-serial testsuite, which uses unix sockets and does
open/close/read/write/poll calls on the sockets.  It all works fine.

Please review and apply.

Amit Shah (7):
  iohandlers: Mark current implementation as 'old'
  iohandlers: Introduce a new API
  char: tcp: Use new iohandler api
  char: udp: Use new iohandler api
  char: fd: Use new iohandler api
  char: stdio: Use new iohandler api
  char: pty: Use new iohandler api

 iohandler.h |   28 +++++++++++
 qemu-char.c |  140 +++++++++++++++++++++++++++++++++++++++++++----------
 vl.c        |  154 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 291 insertions(+), 31 deletions(-)
 create mode 100644 iohandler.h

-- 
1.7.4

[Qemu-devel] [PATCH 1/7] iohandlers: Mark current implementation as 'old'

[Amit Shah]

Mark the current iohandler list as 'old'.  In the next commit we'll
introduce a new iohandler api that will replace the list name.

The 'old' list will eventually be completely replaced by the new
implementation.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 vl.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/vl.c b/vl.c
index b436952..e248ec4 100644
--- a/vl.c
+++ b/vl.c
@@ -1037,8 +1037,8 @@ typedef struct IOHandlerRecord {
     QLIST_ENTRY(IOHandlerRecord) next;
 } IOHandlerRecord;
 
-static QLIST_HEAD(, IOHandlerRecord) io_handlers =
-    QLIST_HEAD_INITIALIZER(io_handlers);
+static QLIST_HEAD(, IOHandlerRecord) io_handlers_old =
+    QLIST_HEAD_INITIALIZER(io_handlers_old);
 
 
 /* XXX: fd_read_poll should be suppressed, but an API change is
@@ -1052,19 +1052,19 @@ int qemu_set_fd_handler2(int fd,
     IOHandlerRecord *ioh;
 
     if (!fd_read && !fd_write) {
-        QLIST_FOREACH(ioh, &io_handlers, next) {
+        QLIST_FOREACH(ioh, &io_handlers_old, next) {
             if (ioh->fd == fd) {
                 ioh->deleted = 1;
                 break;
             }
         }
     } else {
-        QLIST_FOREACH(ioh, &io_handlers, next) {
+        QLIST_FOREACH(ioh, &io_handlers_old, next) {
             if (ioh->fd == fd)
                 goto found;
         }
         ioh = qemu_mallocz(sizeof(IOHandlerRecord));
-        QLIST_INSERT_HEAD(&io_handlers, ioh, next);
+        QLIST_INSERT_HEAD(&io_handlers_old, ioh, next);
     found:
         ioh->fd = fd;
         ioh->fd_read_poll = fd_read_poll;
@@ -1347,7 +1347,7 @@ void main_loop_wait(int nonblocking)
     FD_ZERO(&rfds);
     FD_ZERO(&wfds);
     FD_ZERO(&xfds);
-    QLIST_FOREACH(ioh, &io_handlers, next) {
+    QLIST_FOREACH(ioh, &io_handlers_old, next) {
         if (ioh->deleted)
             continue;
         if (ioh->fd_read &&
@@ -1375,7 +1375,7 @@ void main_loop_wait(int nonblocking)
     if (ret > 0) {
         IOHandlerRecord *pioh;
 
-        QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
+        QLIST_FOREACH_SAFE(ioh, &io_handlers_old, next, pioh) {
             if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) {
                 ioh->fd_read(ioh->opaque);
             }
-- 
1.7.4

[Qemu-devel] [PATCH 2/7] iohandlers: Introduce a new API

[Amit Shah]

Introduce a new iohandler api that doesn't have multiple callbacks.
Instead, a single callback and a mask of events that got set will be
passed on to the handler.  This will ease our transition to a poll()
interface instead of the current select() that happens on the fds.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 iohandler.h |   28 +++++++++++
 vl.c        |  144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 172 insertions(+), 0 deletions(-)
 create mode 100644 iohandler.h

diff --git a/iohandler.h b/iohandler.h
new file mode 100644
index 0000000..6553835
--- /dev/null
+++ b/iohandler.h
@@ -0,0 +1,28 @@
+/*
+ * Declarations for iohandler functions
+ *
+ * Copyright Red Hat, Inc. 2011
+ * Copyright Amit Shah <amit.shah@redhat.com> 2011
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef _QEMU_IOHANDLER_H
+#define _QEMU_IOHANDLER_H
+
+/* iohandler masks */
+#define IOH_MASK_CAN_READ	(1U << 0)
+#define IOH_MASK_READ		(1U << 1)
+#define IOH_MASK_WRITE		(1U << 2)
+
+typedef int IOAllHandler(void *opaque, unsigned int mask);
+
+int assign_iohandler(int fd, IOAllHandler *handler, unsigned int mask,
+                     void *opaque);
+int remove_iohandler(int fd);
+int update_fd_mask(int fd, unsigned int mask);
+int get_fd_mask(int fd, unsigned int *mask);
+
+
+#endif /* _QEMU_IOHANDLER_H */
diff --git a/vl.c b/vl.c
index e248ec4..9dabee2 100644
--- a/vl.c
+++ b/vl.c
@@ -2,6 +2,8 @@
  * QEMU System Emulator
  *
  * Copyright (c) 2003-2008 Fabrice Bellard
+ * Copyright (C) 2011 Red Hat, Inc.
+ * Copyright (C) 2011 Amit Shah, <amit.shah@redhat.com>
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -136,6 +138,7 @@ int main(int argc, char **argv)
 #include "gdbstub.h"
 #include "qemu-timer.h"
 #include "qemu-char.h"
+#include "iohandler.h"
 #include "cache-utils.h"
 #include "block.h"
 #include "blockdev.h"
@@ -1030,6 +1033,8 @@ typedef struct IOHandlerRecord {
     IOCanReadHandler *fd_read_poll;
     IOHandler *fd_read;
     IOHandler *fd_write;
+    IOAllHandler *fd_iohandler;
+    unsigned int mask;
     int deleted;
     void *opaque;
     /* temporary data */
@@ -1040,6 +1045,97 @@ typedef struct IOHandlerRecord {
 static QLIST_HEAD(, IOHandlerRecord) io_handlers_old =
     QLIST_HEAD_INITIALIZER(io_handlers_old);
 
+static QLIST_HEAD(, IOHandlerRecord) io_handlers =
+    QLIST_HEAD_INITIALIZER(io_handlers);
+
+static IOHandlerRecord *find_iohandler(int fd)
+{
+    IOHandlerRecord *ioh;
+
+    QLIST_FOREACH(ioh, &io_handlers, next) {
+        if (ioh->fd == fd) {
+            return ioh;
+        }
+    }
+    return NULL;
+}
+
+static int update_iohandler_mask(IOHandlerRecord *ioh, unsigned int mask)
+{
+    assert(ioh);
+
+    ioh->mask = mask;
+    return 0;
+}
+
+/*
+ * Updates the mask for the fd.  Returns 0 on success.
+ */
+int update_fd_mask(int fd, unsigned int mask)
+{
+    IOHandlerRecord *ioh;
+
+    ioh = find_iohandler(fd);
+    if (!ioh) {
+        return -1;
+    }
+
+    return update_iohandler_mask(ioh, mask);
+}
+
+/*
+ * Populates the mask registered in *mask.  Only valid if return value
+ * is 0.
+ */
+int get_fd_mask(int fd, unsigned int *mask)
+{
+    IOHandlerRecord *ioh;
+
+    ioh = find_iohandler(fd);
+    if (!ioh) {
+        return -1;
+    }
+    *mask = ioh->mask;
+
+    return 0;
+}
+
+/*
+ * Assign an iohandler to an fd.  Can also be used to update the mask
+ * or handler or the opaque pointer and also to delete the iohandler.
+ * Returns 0 on success.
+ */
+int assign_iohandler(int fd, IOAllHandler *handler, unsigned int mask,
+                     void *opaque)
+{
+    IOHandlerRecord *ioh;
+
+    ioh = find_iohandler(fd);
+
+    if (!handler) {
+        if (ioh) {
+            ioh->deleted = 1;
+        }
+        return 0;
+    }
+
+    if (!ioh) {
+        ioh = qemu_mallocz(sizeof(IOHandlerRecord));
+        QLIST_INSERT_HEAD(&io_handlers, ioh, next);
+        ioh->fd = fd;
+    }
+    ioh->fd_iohandler = handler;
+    ioh->opaque = opaque;
+    update_iohandler_mask(ioh, mask);
+    ioh->deleted = 0;
+
+    return 0;
+}
+
+int remove_iohandler(int fd)
+{
+    return assign_iohandler(fd, NULL, 0, NULL);
+}
 
 /* XXX: fd_read_poll should be suppressed, but an API change is
    necessary in the character devices to suppress fd_can_read(). */
@@ -1363,6 +1459,26 @@ void main_loop_wait(int nonblocking)
                 nfds = ioh->fd;
         }
     }
+    QLIST_FOREACH(ioh, &io_handlers, next) {
+        if (ioh->deleted) {
+            continue;
+        }
+        if (ioh->mask & IOH_MASK_READ &&
+            (!(ioh->mask & IOH_MASK_CAN_READ) ||
+             (ioh->fd_iohandler(ioh->opaque, IOH_MASK_CAN_READ) != 0))) {
+
+            FD_SET(ioh->fd, &rfds);
+            if (ioh->fd > nfds) {
+                nfds = ioh->fd;
+            }
+        }
+        if (ioh->mask & IOH_MASK_WRITE) {
+            FD_SET(ioh->fd, &wfds);
+            if (ioh->fd > nfds) {
+                nfds = ioh->fd;
+            }
+        }
+    }
 
     tv.tv_sec = timeout / 1000;
     tv.tv_usec = (timeout % 1000) * 1000;
@@ -1389,6 +1505,34 @@ void main_loop_wait(int nonblocking)
                 qemu_free(ioh);
             }
         }
+        QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
+            unsigned int mask;
+
+            mask = 0;
+            if (!ioh->deleted) {
+                if (FD_ISSET(ioh->fd, &rfds)) {
+                    mask |= IOH_MASK_READ;
+                }
+                if (FD_ISSET(ioh->fd, &wfds)) {
+                    mask |= IOH_MASK_WRITE;
+                }
+            }
+            /*
+             * Strip out flags the caller didn't ask for -- shouldn't
+             * happen since we set rfds and wfds based on the value of
+             * ioh->mask anyway.
+             */
+            mask &= ioh->mask;
+            if (mask) {
+                ioh->fd_iohandler(ioh->opaque, mask);
+            }
+            /* Do this last in case the handler marked it for deletion */
+            if (ioh->deleted) {
+                QLIST_REMOVE(ioh, next);
+                qemu_free(ioh);
+                continue;
+            }
+        }
     }
 
     slirp_select_poll(&rfds, &wfds, &xfds, (ret < 0));
-- 
1.7.4

[Qemu-devel] [PATCH 3/7] char: tcp: Use new iohandler api

[Amit Shah]

Update the tcp code to use the new iohandler api.  The change is mostly
mechanical with a new iohandler function calling the older functions
depending on the value of the 'mask' field.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c |   48 ++++++++++++++++++++++++++++++++++++------------
 1 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index bd4e944..ba58c18 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -21,6 +21,7 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
+#include "iohandler.h"
 #include "qemu-common.h"
 #include "net.h"
 #include "monitor.h"
@@ -1909,7 +1910,7 @@ typedef struct {
     int msgfd;
 } TCPCharDriver;
 
-static void tcp_chr_accept(void *opaque);
+static int tcp_accept_handler(void *opaque, unsigned int mask);
 
 static int tcp_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
@@ -2064,9 +2065,10 @@ static void tcp_chr_read(void *opaque)
         /* connection closed */
         s->connected = 0;
         if (s->listen_fd >= 0) {
-            qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr);
+            assign_iohandler(s->listen_fd, tcp_accept_handler, IOH_MASK_READ,
+                             chr);
         }
-        qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+        remove_iohandler(s->fd);
         closesocket(s->fd);
         s->fd = -1;
         qemu_chr_event(chr, CHR_EVENT_CLOSED);
@@ -2078,6 +2080,22 @@ static void tcp_chr_read(void *opaque)
     }
 }
 
+static int tcp_iohandler(void *opaque, unsigned int mask)
+{
+    int ret;
+
+    ret = 0;
+    switch(mask) {
+    case IOH_MASK_CAN_READ:
+        ret = tcp_chr_read_poll(opaque);
+        break;
+    case IOH_MASK_READ:
+        tcp_chr_read(opaque);
+        break;
+    }
+    return ret;
+}
+
 #ifndef _WIN32
 CharDriverState *qemu_chr_open_eventfd(int eventfd)
 {
@@ -2091,8 +2109,8 @@ static void tcp_chr_connect(void *opaque)
     TCPCharDriver *s = chr->opaque;
 
     s->connected = 1;
-    qemu_set_fd_handler2(s->fd, tcp_chr_read_poll,
-                         tcp_chr_read, NULL, chr);
+    assign_iohandler(s->fd, tcp_iohandler, IOH_MASK_CAN_READ|IOH_MASK_READ,
+                     chr);
     qemu_chr_generic_open(chr);
 }
 
@@ -2117,7 +2135,7 @@ static void socket_set_nodelay(int fd)
     setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (char *)&val, sizeof(val));
 }
 
-static void tcp_chr_accept(void *opaque)
+static int tcp_accept_handler(void *opaque, unsigned int mask)
 {
     CharDriverState *chr = opaque;
     TCPCharDriver *s = chr->opaque;
@@ -2129,6 +2147,10 @@ static void tcp_chr_accept(void *opaque)
     socklen_t len;
     int fd;
 
+    if (!(mask & IOH_MASK_READ)) {
+        return 0;
+    }
+
     for(;;) {
 #ifndef _WIN32
 	if (s->is_unix) {
@@ -2142,7 +2164,7 @@ static void tcp_chr_accept(void *opaque)
 	}
         fd = qemu_accept(s->listen_fd, addr, &len);
         if (fd < 0 && errno != EINTR) {
-            return;
+            return 0;
         } else if (fd >= 0) {
             if (s->do_telnetopt)
                 tcp_chr_telnet_init(fd);
@@ -2153,19 +2175,21 @@ static void tcp_chr_accept(void *opaque)
     if (s->do_nodelay)
         socket_set_nodelay(fd);
     s->fd = fd;
-    qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
+    remove_iohandler(s->listen_fd);
     tcp_chr_connect(chr);
+
+    return 0;
 }
 
 static void tcp_chr_close(CharDriverState *chr)
 {
     TCPCharDriver *s = chr->opaque;
     if (s->fd >= 0) {
-        qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+        remove_iohandler(s->fd);
         closesocket(s->fd);
     }
     if (s->listen_fd >= 0) {
-        qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
+        remove_iohandler(s->listen_fd);
         closesocket(s->listen_fd);
     }
     qemu_free(s);
@@ -2227,7 +2251,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
 
     if (is_listen) {
         s->listen_fd = fd;
-        qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr);
+        assign_iohandler(s->listen_fd, tcp_accept_handler, IOH_MASK_READ, chr);
         if (is_telnet)
             s->do_telnetopt = 1;
 
@@ -2257,7 +2281,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     if (is_listen && is_waitconnect) {
         printf("QEMU waiting for connection on: %s\n",
                chr->filename);
-        tcp_chr_accept(chr);
+        tcp_accept_handler(chr, IOH_MASK_READ);
         socket_set_nonblock(s->listen_fd);
     }
     return chr;
-- 
1.7.4

[Qemu-devel] [PATCH 4/7] char: udp: Use new iohandler api

[Amit Shah]

Update the udp code to use the new iohandler api.  The change is mostly
mechanical with a new iohandler function calling the older functions
depending on the value of the 'mask' field.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index ba58c18..61f8358 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1842,13 +1842,29 @@ static void udp_chr_read(void *opaque)
     }
 }
 
+static int udp_iohandler(void *opaque, unsigned int mask)
+{
+    int ret;
+
+    ret = 0;
+    switch(mask) {
+    case IOH_MASK_CAN_READ:
+        ret = udp_chr_read_poll(opaque);
+        break;
+    case IOH_MASK_READ:
+        udp_chr_read(opaque);
+        break;
+    }
+    return ret;
+}
+
 static void udp_chr_update_read_handler(CharDriverState *chr)
 {
     NetCharDriver *s = chr->opaque;
 
     if (s->fd >= 0) {
-        qemu_set_fd_handler2(s->fd, udp_chr_read_poll,
-                             udp_chr_read, NULL, chr);
+        assign_iohandler(s->fd, udp_iohandler,
+                          IOH_MASK_CAN_READ|IOH_MASK_READ, chr);
     }
 }
 
@@ -1856,7 +1872,7 @@ static void udp_chr_close(CharDriverState *chr)
 {
     NetCharDriver *s = chr->opaque;
     if (s->fd >= 0) {
-        qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+        remove_iohandler(s->fd);
         closesocket(s->fd);
     }
     qemu_free(s);
-- 
1.7.4

[Qemu-devel] [PATCH 5/7] char: fd: Use new iohandler api

[Amit Shah]

Update the fd code to use the new iohandler api.  The change is mostly
mechanical with a new iohandler function calling the older functions
depending on the value of the 'mask' field.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c |   24 ++++++++++++++++++++----
 1 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 61f8358..d93820e 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -572,7 +572,7 @@ static void fd_chr_read(void *opaque)
     size = read(s->fd_in, buf, len);
     if (size == 0) {
         /* FD has been closed. Remove it from the active list.  */
-        qemu_set_fd_handler2(s->fd_in, NULL, NULL, NULL, NULL);
+        remove_iohandler(s->fd_in);
         qemu_chr_event(chr, CHR_EVENT_CLOSED);
         return;
     }
@@ -581,6 +581,22 @@ static void fd_chr_read(void *opaque)
     }
 }
 
+static int fd_iohandler(void *opaque, unsigned int mask)
+{
+    int ret;
+
+    ret = 0;
+    switch(mask) {
+    case IOH_MASK_CAN_READ:
+        ret = fd_chr_read_poll(opaque);
+        break;
+    case IOH_MASK_READ:
+        fd_chr_read(opaque);
+        break;
+    }
+    return ret;
+}
+
 static void fd_chr_update_read_handler(CharDriverState *chr)
 {
     FDCharDriver *s = chr->opaque;
@@ -588,8 +604,8 @@ static void fd_chr_update_read_handler(CharDriverState *chr)
     if (s->fd_in >= 0) {
         if (display_type == DT_NOGRAPHIC && s->fd_in == 0) {
         } else {
-            qemu_set_fd_handler2(s->fd_in, fd_chr_read_poll,
-                                 fd_chr_read, NULL, chr);
+            assign_iohandler(s->fd_in, fd_iohandler,
+                             IOH_MASK_CAN_READ|IOH_MASK_READ, chr);
         }
     }
 }
@@ -601,7 +617,7 @@ static void fd_chr_close(struct CharDriverState *chr)
     if (s->fd_in >= 0) {
         if (display_type == DT_NOGRAPHIC && s->fd_in == 0) {
         } else {
-            qemu_set_fd_handler2(s->fd_in, NULL, NULL, NULL, NULL);
+            remove_iohandler(s->fd_in);
         }
     }
 
-- 
1.7.4

[Qemu-devel] [PATCH 6/7] char: stdio: Use new iohandler api

[Amit Shah]

Update the stdio code to use the new iohandler api.  The change is
mostly mechanical with a new iohandler function calling the older
functions depending on the value of the 'mask' field.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index d93820e..ade28ba 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -717,7 +717,7 @@ static void stdio_read(void *opaque)
     size = read(0, buf, 1);
     if (size == 0) {
         /* stdin has been closed. Remove it from the active list.  */
-        qemu_set_fd_handler2(0, NULL, NULL, NULL, NULL);
+        remove_iohandler(0);
         qemu_chr_event(chr, CHR_EVENT_CLOSED);
         return;
     }
@@ -730,6 +730,22 @@ static void stdio_read(void *opaque)
     }
 }
 
+static int stdio_iohandler(void *opaque, unsigned int mask)
+{
+    int ret;
+
+    ret = 0;
+    switch(mask) {
+    case IOH_MASK_CAN_READ:
+        ret = stdio_read_poll(opaque);
+        break;
+    case IOH_MASK_READ:
+        stdio_read(opaque);
+        break;
+    }
+    return ret;
+}
+
 /* init terminal so that we can grab keys */
 static struct termios oldtty;
 static int old_fd0_flags;
@@ -767,7 +783,7 @@ static void qemu_chr_close_stdio(struct CharDriverState *chr)
 {
     term_exit();
     stdio_nb_clients--;
-    qemu_set_fd_handler2(0, NULL, NULL, NULL, NULL);
+    remove_iohandler(0);
     fd_chr_close(chr);
 }
 
@@ -787,7 +803,7 @@ static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts)
     chr = qemu_chr_open_fd(0, 1);
     chr->chr_close = qemu_chr_close_stdio;
     chr->chr_set_echo = qemu_chr_set_echo_stdio;
-    qemu_set_fd_handler2(0, stdio_read_poll, stdio_read, NULL, chr);
+    assign_iohandler(0, stdio_iohandler, IOH_MASK_CAN_READ|IOH_MASK_READ, chr);
     stdio_nb_clients++;
     stdio_allow_signal = qemu_opt_get_bool(opts, "signal",
                                            display_type != DT_NOGRAPHIC);
-- 
1.7.4

[Qemu-devel] [PATCH 7/7] char: pty: Use new iohandler api

[Amit Shah]

Update the pty code to use the new iohandler api.  The change is mostly
mechanical with a new iohandler function calling the older functions
depending on the value of the 'mask' field.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c |   24 ++++++++++++++++++++----
 1 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index ade28ba..6875b00 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -929,12 +929,28 @@ static void pty_chr_read(void *opaque)
     }
 }
 
+static int pty_iohandler(void *opaque, unsigned int mask)
+{
+    int ret;
+
+    ret = 0;
+    switch(mask) {
+    case IOH_MASK_CAN_READ:
+        ret = pty_chr_read_poll(opaque);
+        break;
+    case IOH_MASK_READ:
+        pty_chr_read(opaque);
+        break;
+    }
+    return ret;
+}
+
 static void pty_chr_update_read_handler(CharDriverState *chr)
 {
     PtyCharDriver *s = chr->opaque;
 
-    qemu_set_fd_handler2(s->fd, pty_chr_read_poll,
-                         pty_chr_read, NULL, chr);
+    assign_iohandler(s->fd, pty_iohandler, IOH_MASK_CAN_READ|IOH_MASK_READ,
+                     chr);
     s->polling = 1;
     /*
      * Short timeout here: just need wait long enougth that qemu makes
@@ -952,7 +968,7 @@ static void pty_chr_state(CharDriverState *chr, int connected)
     PtyCharDriver *s = chr->opaque;
 
     if (!connected) {
-        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+        remove_iohandler(s->fd);
         s->connected = 0;
         s->polling = 0;
         /* (re-)connect poll interval for idle guests: once per second.
@@ -988,7 +1004,7 @@ static void pty_chr_close(struct CharDriverState *chr)
 {
     PtyCharDriver *s = chr->opaque;
 
-    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+    remove_iohandler(s->fd);
     close(s->fd);
     qemu_del_timer(s->timer);
     qemu_free_timer(s->timer);
-- 
1.7.4

Re: [Qemu-devel] [PATCH 1/7] iohandlers: Mark current implementation as 'old'

[Avi Kivity]

On 02/22/2011 12:18 PM, Amit Shah wrote:
> Mark the current iohandler list as 'old'.  In the next commit we'll
> introduce a new iohandler api that will replace the list name.
>
> The 'old' list will eventually be completely replaced by the new
> implementation.
>

A better way to do this is to implement the old API in terms of the new 
API.  This ensures you don't lose any functionality, and reduces the 
amount of low-level infrastructure.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/7] iohandlers: Mark current implementation as 'old'

[Amit Shah]

On (Tue) 22 Feb 2011 [13:09:34], Avi Kivity wrote:
> On 02/22/2011 12:18 PM, Amit Shah wrote:
> >Mark the current iohandler list as 'old'.  In the next commit we'll
> >introduce a new iohandler api that will replace the list name.
> >
> >The 'old' list will eventually be completely replaced by the new
> >implementation.
> 
> A better way to do this is to implement the old API in terms of the
> new API.  This ensures you don't lose any functionality, and reduces
> the amount of low-level infrastructure.

With this new approach of switching to just one callback, that is more
work for little gain as we'll just deprecate the older API soon.  (The
previous patches used this approach, btw.)

Would you still prefer to have an old->new mapping?

		Amit

Re: [Qemu-devel] [PATCH 2/7] iohandlers: Introduce a new API

[Alon Levy]

On Tue, Feb 22, 2011 at 03:48:31PM +0530, Amit Shah wrote:
> Introduce a new iohandler api that doesn't have multiple callbacks.
> Instead, a single callback and a mask of events that got set will be
> passed on to the handler.  This will ease our transition to a poll()
> interface instead of the current select() that happens on the fds.
> 

Is the mask argument to the callback indicative of a real mask, or is
this an enum when passed to the clients? i.e. you seem to use a switch
in all handlers, so is it actually meant that each callback has a single
possible value, not an or'ed one?

> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  iohandler.h |   28 +++++++++++
>  vl.c        |  144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 172 insertions(+), 0 deletions(-)
>  create mode 100644 iohandler.h
> 
> diff --git a/iohandler.h b/iohandler.h
> new file mode 100644
> index 0000000..6553835
> --- /dev/null
> +++ b/iohandler.h
> @@ -0,0 +1,28 @@
> +/*
> + * Declarations for iohandler functions
> + *
> + * Copyright Red Hat, Inc. 2011
> + * Copyright Amit Shah <amit.shah@redhat.com> 2011
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef _QEMU_IOHANDLER_H
> +#define _QEMU_IOHANDLER_H
> +
> +/* iohandler masks */
> +#define IOH_MASK_CAN_READ	(1U << 0)
> +#define IOH_MASK_READ		(1U << 1)
> +#define IOH_MASK_WRITE		(1U << 2)
> +
> +typedef int IOAllHandler(void *opaque, unsigned int mask);
> +
> +int assign_iohandler(int fd, IOAllHandler *handler, unsigned int mask,
> +                     void *opaque);
> +int remove_iohandler(int fd);
> +int update_fd_mask(int fd, unsigned int mask);
> +int get_fd_mask(int fd, unsigned int *mask);
> +
> +
> +#endif /* _QEMU_IOHANDLER_H */
> diff --git a/vl.c b/vl.c
> index e248ec4..9dabee2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2,6 +2,8 @@
>   * QEMU System Emulator
>   *
>   * Copyright (c) 2003-2008 Fabrice Bellard
> + * Copyright (C) 2011 Red Hat, Inc.
> + * Copyright (C) 2011 Amit Shah, <amit.shah@redhat.com>
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>   * of this software and associated documentation files (the "Software"), to deal
> @@ -136,6 +138,7 @@ int main(int argc, char **argv)
>  #include "gdbstub.h"
>  #include "qemu-timer.h"
>  #include "qemu-char.h"
> +#include "iohandler.h"
>  #include "cache-utils.h"
>  #include "block.h"
>  #include "blockdev.h"
> @@ -1030,6 +1033,8 @@ typedef struct IOHandlerRecord {
>      IOCanReadHandler *fd_read_poll;
>      IOHandler *fd_read;
>      IOHandler *fd_write;
> +    IOAllHandler *fd_iohandler;
> +    unsigned int mask;
>      int deleted;
>      void *opaque;
>      /* temporary data */
> @@ -1040,6 +1045,97 @@ typedef struct IOHandlerRecord {
>  static QLIST_HEAD(, IOHandlerRecord) io_handlers_old =
>      QLIST_HEAD_INITIALIZER(io_handlers_old);
>  
> +static QLIST_HEAD(, IOHandlerRecord) io_handlers =
> +    QLIST_HEAD_INITIALIZER(io_handlers);
> +
> +static IOHandlerRecord *find_iohandler(int fd)
> +{
> +    IOHandlerRecord *ioh;
> +
> +    QLIST_FOREACH(ioh, &io_handlers, next) {
> +        if (ioh->fd == fd) {
> +            return ioh;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static int update_iohandler_mask(IOHandlerRecord *ioh, unsigned int mask)
> +{
> +    assert(ioh);
> +
> +    ioh->mask = mask;
> +    return 0;
> +}
> +
> +/*
> + * Updates the mask for the fd.  Returns 0 on success.
> + */
> +int update_fd_mask(int fd, unsigned int mask)
> +{
> +    IOHandlerRecord *ioh;
> +
> +    ioh = find_iohandler(fd);
> +    if (!ioh) {
> +        return -1;
> +    }
> +
> +    return update_iohandler_mask(ioh, mask);
> +}
> +
> +/*
> + * Populates the mask registered in *mask.  Only valid if return value
> + * is 0.
> + */
> +int get_fd_mask(int fd, unsigned int *mask)
> +{
> +    IOHandlerRecord *ioh;
> +
> +    ioh = find_iohandler(fd);
> +    if (!ioh) {
> +        return -1;
> +    }
> +    *mask = ioh->mask;
> +
> +    return 0;
> +}
> +
> +/*
> + * Assign an iohandler to an fd.  Can also be used to update the mask
> + * or handler or the opaque pointer and also to delete the iohandler.
> + * Returns 0 on success.
> + */
> +int assign_iohandler(int fd, IOAllHandler *handler, unsigned int mask,
> +                     void *opaque)
> +{
> +    IOHandlerRecord *ioh;
> +
> +    ioh = find_iohandler(fd);
> +
> +    if (!handler) {
> +        if (ioh) {
> +            ioh->deleted = 1;
> +        }
> +        return 0;
> +    }
> +
> +    if (!ioh) {
> +        ioh = qemu_mallocz(sizeof(IOHandlerRecord));
> +        QLIST_INSERT_HEAD(&io_handlers, ioh, next);
> +        ioh->fd = fd;
> +    }
> +    ioh->fd_iohandler = handler;
> +    ioh->opaque = opaque;
> +    update_iohandler_mask(ioh, mask);
> +    ioh->deleted = 0;
> +
> +    return 0;
> +}
> +
> +int remove_iohandler(int fd)
> +{
> +    return assign_iohandler(fd, NULL, 0, NULL);
> +}
>  
>  /* XXX: fd_read_poll should be suppressed, but an API change is
>     necessary in the character devices to suppress fd_can_read(). */
> @@ -1363,6 +1459,26 @@ void main_loop_wait(int nonblocking)
>                  nfds = ioh->fd;
>          }
>      }
> +    QLIST_FOREACH(ioh, &io_handlers, next) {
> +        if (ioh->deleted) {
> +            continue;
> +        }
> +        if (ioh->mask & IOH_MASK_READ &&
> +            (!(ioh->mask & IOH_MASK_CAN_READ) ||
> +             (ioh->fd_iohandler(ioh->opaque, IOH_MASK_CAN_READ) != 0))) {
> +
> +            FD_SET(ioh->fd, &rfds);
> +            if (ioh->fd > nfds) {
> +                nfds = ioh->fd;
> +            }
> +        }
> +        if (ioh->mask & IOH_MASK_WRITE) {
> +            FD_SET(ioh->fd, &wfds);
> +            if (ioh->fd > nfds) {
> +                nfds = ioh->fd;
> +            }
> +        }
> +    }
>  
>      tv.tv_sec = timeout / 1000;
>      tv.tv_usec = (timeout % 1000) * 1000;
> @@ -1389,6 +1505,34 @@ void main_loop_wait(int nonblocking)
>                  qemu_free(ioh);
>              }
>          }
> +        QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
> +            unsigned int mask;
> +
> +            mask = 0;
> +            if (!ioh->deleted) {
> +                if (FD_ISSET(ioh->fd, &rfds)) {
> +                    mask |= IOH_MASK_READ;
> +                }
> +                if (FD_ISSET(ioh->fd, &wfds)) {
> +                    mask |= IOH_MASK_WRITE;
> +                }
> +            }
> +            /*
> +             * Strip out flags the caller didn't ask for -- shouldn't
> +             * happen since we set rfds and wfds based on the value of
> +             * ioh->mask anyway.
> +             */
> +            mask &= ioh->mask;
> +            if (mask) {
> +                ioh->fd_iohandler(ioh->opaque, mask);
> +            }
> +            /* Do this last in case the handler marked it for deletion */
> +            if (ioh->deleted) {
> +                QLIST_REMOVE(ioh, next);
> +                qemu_free(ioh);
> +                continue;
> +            }
> +        }
>      }
>  
>      slirp_select_poll(&rfds, &wfds, &xfds, (ret < 0));
> -- 
> 1.7.4
> 
> 

Re: [Qemu-devel] [PATCH 2/7] iohandlers: Introduce a new API

[Amit Shah]

On (Tue) 22 Feb 2011 [14:39:03], Alon Levy wrote:
> On Tue, Feb 22, 2011 at 03:48:31PM +0530, Amit Shah wrote:
> > Introduce a new iohandler api that doesn't have multiple callbacks.
> > Instead, a single callback and a mask of events that got set will be
> > passed on to the handler.  This will ease our transition to a poll()
> > interface instead of the current select() that happens on the fds.
> 
> Is the mask argument to the callback indicative of a real mask, or is
> this an enum when passed to the clients? i.e. you seem to use a switch
> in all handlers, so is it actually meant that each callback has a single
> possible value, not an or'ed one?

Hm, this will actually be a bug when we get rid of the _can_read
callbacks.  So the switch will have to change to something else for
sure.

		Amit

Re: [Qemu-devel] [PATCH 2/7] iohandlers: Introduce a new API

[Avi Kivity]

On 02/22/2011 12:18 PM, Amit Shah wrote:
> Introduce a new iohandler api that doesn't have multiple callbacks.
> Instead, a single callback and a mask of events that got set will be
> passed on to the handler.  This will ease our transition to a poll()
> interface instead of the current select() that happens on the fds.
>
>
> +
> +/* iohandler masks */
> +#define IOH_MASK_CAN_READ	(1U<<  0)
> +#define IOH_MASK_READ		(1U<<  1)
> +#define IOH_MASK_WRITE		(1U<<  2)
> +
> +typedef int IOAllHandler(void *opaque, unsigned int mask);

Strange name.

Drop the opaque, instead put the IOHandler in there (or maybe the 
CharDev?) and use container_of().

> +
> +int assign_iohandler(int fd, IOAllHandler *handler, unsigned int mask,
> +                     void *opaque);
> +int remove_iohandler(int fd);
> +int update_fd_mask(int fd, unsigned int mask);
> +int get_fd_mask(int fd, unsigned int *mask);
> +

iohandler_init(IOHandler *ioh, int fd, IOEventHandler *handler, unsigned 
mask)
iohandler_del(IOHandler *ioh)
iohandler_set_event_mask(IOHandler *ioh, unsigned mask)
iohandler_event_mask(IOHandler *ioh)

No opaques, use an object as a key so you don't have to search for it.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/7] iohandlers: Mark current implementation as 'old'

[Avi Kivity]

On 02/22/2011 01:17 PM, Amit Shah wrote:
> On (Tue) 22 Feb 2011 [13:09:34], Avi Kivity wrote:
> >  On 02/22/2011 12:18 PM, Amit Shah wrote:
> >  >Mark the current iohandler list as 'old'.  In the next commit we'll
> >  >introduce a new iohandler api that will replace the list name.
> >  >
> >  >The 'old' list will eventually be completely replaced by the new
> >  >implementation.
> >
> >  A better way to do this is to implement the old API in terms of the
> >  new API.  This ensures you don't lose any functionality, and reduces
> >  the amount of low-level infrastructure.
>
> With this new approach of switching to just one callback, that is more
> work for little gain as we'll just deprecate the older API soon.  (The
> previous patches used this approach, btw.)

Does "deprecate" mean "completely remove"?

If so I agree.

> Would you still prefer to have an old->new mapping?
>

If you don't remove the old API, yes.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/7] iohandlers: Mark current implementation as 'old'

[Amit Shah]

On (Tue) 22 Feb 2011 [15:28:59], Avi Kivity wrote:
> On 02/22/2011 01:17 PM, Amit Shah wrote:
> >On (Tue) 22 Feb 2011 [13:09:34], Avi Kivity wrote:
> >>  On 02/22/2011 12:18 PM, Amit Shah wrote:
> >>  >Mark the current iohandler list as 'old'.  In the next commit we'll
> >>  >introduce a new iohandler api that will replace the list name.
> >>  >
> >>  >The 'old' list will eventually be completely replaced by the new
> >>  >implementation.
> >>
> >>  A better way to do this is to implement the old API in terms of the
> >>  new API.  This ensures you don't lose any functionality, and reduces
> >>  the amount of low-level infrastructure.
> >
> >With this new approach of switching to just one callback, that is more
> >work for little gain as we'll just deprecate the older API soon.  (The
> >previous patches used this approach, btw.)
> 
> Does "deprecate" mean "completely remove"?
> 
> If so I agree.

Yes, completely removing the older API is the plan (as noted in
message 00).

		Amit

Re: [Qemu-devel] [PATCH 2/7] iohandlers: Introduce a new API

[Amit Shah]

On (Tue) 22 Feb 2011 [15:28:02], Avi Kivity wrote:
> On 02/22/2011 12:18 PM, Amit Shah wrote:
> >Introduce a new iohandler api that doesn't have multiple callbacks.
> >Instead, a single callback and a mask of events that got set will be
> >passed on to the handler.  This will ease our transition to a poll()
> >interface instead of the current select() that happens on the fds.
> >
> >
> >+
> >+/* iohandler masks */
> >+#define IOH_MASK_CAN_READ	(1U<<  0)
> >+#define IOH_MASK_READ		(1U<<  1)
> >+#define IOH_MASK_WRITE		(1U<<  2)
> >+
> >+typedef int IOAllHandler(void *opaque, unsigned int mask);
> 
> Strange name.

IOHandler is taken.  I plan to switch back to IOHandler when the older
one is removed.

> Drop the opaque, instead put the IOHandler in there (or maybe the
> CharDev?) and use container_of().

Can put the IOHandler there; the chardev is just one user of
iohandlers, and there are others.

> >+
> >+int assign_iohandler(int fd, IOAllHandler *handler, unsigned int mask,
> >+                     void *opaque);
> >+int remove_iohandler(int fd);
> >+int update_fd_mask(int fd, unsigned int mask);
> >+int get_fd_mask(int fd, unsigned int *mask);
> >+
> 
> iohandler_init(IOHandler *ioh, int fd, IOEventHandler *handler,
> unsigned mask)
> iohandler_del(IOHandler *ioh)
> iohandler_set_event_mask(IOHandler *ioh, unsigned mask)
> iohandler_event_mask(IOHandler *ioh)
> 
> No opaques, use an object as a key so you don't have to search for it.

Thanks!

		Amit

Re: [Qemu-devel] [PATCH 1/7] iohandlers: Mark current implementation as 'old'

[Avi Kivity]

On 02/22/2011 04:37 PM, Amit Shah wrote:
> On (Tue) 22 Feb 2011 [15:28:59], Avi Kivity wrote:
> >  On 02/22/2011 01:17 PM, Amit Shah wrote:
> >  >On (Tue) 22 Feb 2011 [13:09:34], Avi Kivity wrote:
> >  >>   On 02/22/2011 12:18 PM, Amit Shah wrote:
> >  >>   >Mark the current iohandler list as 'old'.  In the next commit we'll
> >  >>   >introduce a new iohandler api that will replace the list name.
> >  >>   >
> >  >>   >The 'old' list will eventually be completely replaced by the new
> >  >>   >implementation.
> >  >>
> >  >>   A better way to do this is to implement the old API in terms of the
> >  >>   new API.  This ensures you don't lose any functionality, and reduces
> >  >>   the amount of low-level infrastructure.
> >  >
> >  >With this new approach of switching to just one callback, that is more
> >  >work for little gain as we'll just deprecate the older API soon.  (The
> >  >previous patches used this approach, btw.)
> >
> >  Does "deprecate" mean "completely remove"?
> >
> >  If so I agree.
>
> Yes, completely removing the older API is the plan (as noted in
> message 00).

Great.  That's a much better deprecate than "add the new thing and let 
the old thing rot" (which I've been guilty of a few times).

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 2/7] iohandlers: Introduce a new API

[Anthony Liguori]

On 02/22/2011 04:18 AM, Amit Shah wrote:
> Introduce a new iohandler api that doesn't have multiple callbacks.
> Instead, a single callback and a mask of events that got set will be
> passed on to the handler.  This will ease our transition to a poll()
> interface instead of the current select() that happens on the fds.
>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
> ---
>   iohandler.h |   28 +++++++++++
>   vl.c        |  144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 172 insertions(+), 0 deletions(-)
>   create mode 100644 iohandler.h
>
> diff --git a/iohandler.h b/iohandler.h
> new file mode 100644
> index 0000000..6553835
> --- /dev/null
> +++ b/iohandler.h
> @@ -0,0 +1,28 @@
> +/*
> + * Declarations for iohandler functions
> + *
> + * Copyright Red Hat, Inc. 2011
> + * Copyright Amit Shah<amit.shah@redhat.com>  2011
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef _QEMU_IOHANDLER_H
> +#define _QEMU_IOHANDLER_H
> +
> +/* iohandler masks */
> +#define IOH_MASK_CAN_READ	(1U<<  0)
> +#define IOH_MASK_READ		(1U<<  1)
> +#define IOH_MASK_WRITE		(1U<<  2)
> +
> +typedef int IOAllHandler(void *opaque, unsigned int mask);
> +
> +int assign_iohandler(int fd, IOAllHandler *handler, unsigned int mask,
> +                     void *opaque);
> +int remove_iohandler(int fd);
> +int update_fd_mask(int fd, unsigned int mask);
> +int get_fd_mask(int fd, unsigned int *mask);
>    

As I previously suggested, we should return a connection handle instead 
of using an fd as an implicit handle.

Regards,

Anthony Liguori

> +
> +
> +#endif /* _QEMU_IOHANDLER_H */
> diff --git a/vl.c b/vl.c
> index e248ec4..9dabee2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2,6 +2,8 @@
>    * QEMU System Emulator
>    *
>    * Copyright (c) 2003-2008 Fabrice Bellard
> + * Copyright (C) 2011 Red Hat, Inc.
> + * Copyright (C) 2011 Amit Shah,<amit.shah@redhat.com>
>    *
>    * Permission is hereby granted, free of charge, to any person obtaining a copy
>    * of this software and associated documentation files (the "Software"), to deal
> @@ -136,6 +138,7 @@ int main(int argc, char **argv)
>   #include "gdbstub.h"
>   #include "qemu-timer.h"
>   #include "qemu-char.h"
> +#include "iohandler.h"
>   #include "cache-utils.h"
>   #include "block.h"
>   #include "blockdev.h"
> @@ -1030,6 +1033,8 @@ typedef struct IOHandlerRecord {
>       IOCanReadHandler *fd_read_poll;
>       IOHandler *fd_read;
>       IOHandler *fd_write;
> +    IOAllHandler *fd_iohandler;
> +    unsigned int mask;
>       int deleted;
>       void *opaque;
>       /* temporary data */
> @@ -1040,6 +1045,97 @@ typedef struct IOHandlerRecord {
>   static QLIST_HEAD(, IOHandlerRecord) io_handlers_old =
>       QLIST_HEAD_INITIALIZER(io_handlers_old);
>
> +static QLIST_HEAD(, IOHandlerRecord) io_handlers =
> +    QLIST_HEAD_INITIALIZER(io_handlers);
> +
> +static IOHandlerRecord *find_iohandler(int fd)
> +{
> +    IOHandlerRecord *ioh;
> +
> +    QLIST_FOREACH(ioh,&io_handlers, next) {
> +        if (ioh->fd == fd) {
> +            return ioh;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static int update_iohandler_mask(IOHandlerRecord *ioh, unsigned int mask)
> +{
> +    assert(ioh);
> +
> +    ioh->mask = mask;
> +    return 0;
> +}
> +
> +/*
> + * Updates the mask for the fd.  Returns 0 on success.
> + */
> +int update_fd_mask(int fd, unsigned int mask)
> +{
> +    IOHandlerRecord *ioh;
> +
> +    ioh = find_iohandler(fd);
> +    if (!ioh) {
> +        return -1;
> +    }
> +
> +    return update_iohandler_mask(ioh, mask);
> +}
> +
> +/*
> + * Populates the mask registered in *mask.  Only valid if return value
> + * is 0.
> + */
> +int get_fd_mask(int fd, unsigned int *mask)
> +{
> +    IOHandlerRecord *ioh;
> +
> +    ioh = find_iohandler(fd);
> +    if (!ioh) {
> +        return -1;
> +    }
> +    *mask = ioh->mask;
> +
> +    return 0;
> +}
> +
> +/*
> + * Assign an iohandler to an fd.  Can also be used to update the mask
> + * or handler or the opaque pointer and also to delete the iohandler.
> + * Returns 0 on success.
> + */
> +int assign_iohandler(int fd, IOAllHandler *handler, unsigned int mask,
> +                     void *opaque)
> +{
> +    IOHandlerRecord *ioh;
> +
> +    ioh = find_iohandler(fd);
> +
> +    if (!handler) {
> +        if (ioh) {
> +            ioh->deleted = 1;
> +        }
> +        return 0;
> +    }
> +
> +    if (!ioh) {
> +        ioh = qemu_mallocz(sizeof(IOHandlerRecord));
> +        QLIST_INSERT_HEAD(&io_handlers, ioh, next);
> +        ioh->fd = fd;
> +    }
> +    ioh->fd_iohandler = handler;
> +    ioh->opaque = opaque;
> +    update_iohandler_mask(ioh, mask);
> +    ioh->deleted = 0;
> +
> +    return 0;
> +}
> +
> +int remove_iohandler(int fd)
> +{
> +    return assign_iohandler(fd, NULL, 0, NULL);
> +}
>
>   /* XXX: fd_read_poll should be suppressed, but an API change is
>      necessary in the character devices to suppress fd_can_read(). */
> @@ -1363,6 +1459,26 @@ void main_loop_wait(int nonblocking)
>                   nfds = ioh->fd;
>           }
>       }
> +    QLIST_FOREACH(ioh,&io_handlers, next) {
> +        if (ioh->deleted) {
> +            continue;
> +        }
> +        if (ioh->mask&  IOH_MASK_READ&&
> +            (!(ioh->mask&  IOH_MASK_CAN_READ) ||
> +             (ioh->fd_iohandler(ioh->opaque, IOH_MASK_CAN_READ) != 0))) {
> +
> +            FD_SET(ioh->fd,&rfds);
> +            if (ioh->fd>  nfds) {
> +                nfds = ioh->fd;
> +            }
> +        }
> +        if (ioh->mask&  IOH_MASK_WRITE) {
> +            FD_SET(ioh->fd,&wfds);
> +            if (ioh->fd>  nfds) {
> +                nfds = ioh->fd;
> +            }
> +        }
> +    }
>
>       tv.tv_sec = timeout / 1000;
>       tv.tv_usec = (timeout % 1000) * 1000;
> @@ -1389,6 +1505,34 @@ void main_loop_wait(int nonblocking)
>                   qemu_free(ioh);
>               }
>           }
> +        QLIST_FOREACH_SAFE(ioh,&io_handlers, next, pioh) {
> +            unsigned int mask;
> +
> +            mask = 0;
> +            if (!ioh->deleted) {
> +                if (FD_ISSET(ioh->fd,&rfds)) {
> +                    mask |= IOH_MASK_READ;
> +                }
> +                if (FD_ISSET(ioh->fd,&wfds)) {
> +                    mask |= IOH_MASK_WRITE;
> +                }
> +            }
> +            /*
> +             * Strip out flags the caller didn't ask for -- shouldn't
> +             * happen since we set rfds and wfds based on the value of
> +             * ioh->mask anyway.
> +             */
> +            mask&= ioh->mask;
> +            if (mask) {
> +                ioh->fd_iohandler(ioh->opaque, mask);
> +            }
> +            /* Do this last in case the handler marked it for deletion */
> +            if (ioh->deleted) {
> +                QLIST_REMOVE(ioh, next);
> +                qemu_free(ioh);
> +                continue;
> +            }
> +        }
>       }
>
>       slirp_select_poll(&rfds,&wfds,&xfds, (ret<  0));
>    

Re: [Qemu-devel] [PATCH 2/7] iohandlers: Introduce a new API

[Anthony Liguori]

On 02/22/2011 07:28 AM, Avi Kivity wrote:
> On 02/22/2011 12:18 PM, Amit Shah wrote:
>> Introduce a new iohandler api that doesn't have multiple callbacks.
>> Instead, a single callback and a mask of events that got set will be
>> passed on to the handler.  This will ease our transition to a poll()
>> interface instead of the current select() that happens on the fds.
>>
>>
>> +
>> +/* iohandler masks */
>> +#define IOH_MASK_CAN_READ    (1U<<  0)
>> +#define IOH_MASK_READ        (1U<<  1)
>> +#define IOH_MASK_WRITE        (1U<<  2)
>> +
>> +typedef int IOAllHandler(void *opaque, unsigned int mask);
>
> Strange name.
>
> Drop the opaque, instead put the IOHandler in there (or maybe the 
> CharDev?) and use container_of().

You know, I'm not there that this is automatically better.

>> +
>> +int assign_iohandler(int fd, IOAllHandler *handler, unsigned int mask,
>> +                     void *opaque);
>> +int remove_iohandler(int fd);
>> +int update_fd_mask(int fd, unsigned int mask);
>> +int get_fd_mask(int fd, unsigned int *mask);
>> +
>
> iohandler_init(IOHandler *ioh, int fd, IOEventHandler *handler, 
> unsigned mask)
> iohandler_del(IOHandler *ioh)
> iohandler_set_event_mask(IOHandler *ioh, unsigned mask)
> iohandler_event_mask(IOHandler *ioh)
>
> No opaques, use an object as a key so you don't have to search for it.

I don't think there should be a setter/getter for the event mask.  Just 
delete the handler and re-add it.

Why is there an IOHandler and an IOEventHandler argument for init?

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 2/7] iohandlers: Introduce a new API

[Avi Kivity]

On 02/22/2011 05:00 PM, Anthony Liguori wrote:
>>
>> Drop the opaque, instead put the IOHandler in there (or maybe the 
>> CharDev?) and use container_of().
>
>
> You know, I'm not there that this is automatically better.

Why not?  One less unsafe cast.

>
>>> +
>>> +int assign_iohandler(int fd, IOAllHandler *handler, unsigned int mask,
>>> +                     void *opaque);
>>> +int remove_iohandler(int fd);
>>> +int update_fd_mask(int fd, unsigned int mask);
>>> +int get_fd_mask(int fd, unsigned int *mask);
>>> +
>>
>> iohandler_init(IOHandler *ioh, int fd, IOEventHandler *handler, 
>> unsigned mask)
>> iohandler_del(IOHandler *ioh)
>> iohandler_set_event_mask(IOHandler *ioh, unsigned mask)
>> iohandler_event_mask(IOHandler *ioh)
>>
>> No opaques, use an object as a key so you don't have to search for it.
>
> I don't think there should be a setter/getter for the event mask.  
> Just delete the handler and re-add it.
>

It's more epoll() compatible to update it.  I'm fine with reregistering 
though.

> Why is there an IOHandler and an IOEventHandler argument for init?

IOHandler is the object, IOEventHandler is the function call.

Basically this is a 1:1 translation of

class IOHandler {
public:
     IOHandler(int fd, unsigned mask);
     ~IOHandler();
     virtual void io_event_handler() = 0;
     unsigned event_mask();
     void set_event_mask(unsigned mask);
}

class MyClient : private IOHandler {
     virtual void io_event_handler();
}

where the compiler automatically generates the container_of() to get to 
the actual MyClient pointer.

Re: [Qemu-devel] [PATCH 2/7] iohandlers: Introduce a new API

[Anthony Liguori]

On 02/22/2011 10:21 AM, Avi Kivity wrote:
> On 02/22/2011 05:00 PM, Anthony Liguori wrote:
>>>
>>> Drop the opaque, instead put the IOHandler in there (or maybe the 
>>> CharDev?) and use container_of().
>>
>>
>> You know, I'm not there that this is automatically better.
>
> Why not?  One less unsafe cast.

It turns out there are a lot of cases in QEMU where there isn't an 
obvious data structure to piggy back on.

>> Why is there an IOHandler and an IOEventHandler argument for init?

So wouldn't you set the IOEventHandler directly in the IOHandler as part 
of initialization?  IOW:

struct MyDevice {
     IOHandler iohandler;
};

iohandler.func = my_io_function;
io_handler_init(&iohandler, fd, event_mask);

That's the paradigm I'm used to at least.

Regards,

Anthony Liguori

> IOHandler is the object, IOEventHandler is the function call.
>
> Basically this is a 1:1 translation of
>
> class IOHandler {
> public:
>     IOHandler(int fd, unsigned mask);
>     ~IOHandler();
>     virtual void io_event_handler() = 0;
>     unsigned event_mask();
>     void set_event_mask(unsigned mask);
> }
>
> class MyClient : private IOHandler {
>     virtual void io_event_handler();
> }
>
> where the compiler automatically generates the container_of() to get 
> to the actual MyClient pointer.
>

Re: [Qemu-devel] [PATCH 2/7] iohandlers: Introduce a new API

[Avi Kivity]

On 02/22/2011 08:21 PM, Anthony Liguori wrote:
> On 02/22/2011 10:21 AM, Avi Kivity wrote:
>> On 02/22/2011 05:00 PM, Anthony Liguori wrote:
>>>>
>>>> Drop the opaque, instead put the IOHandler in there (or maybe the 
>>>> CharDev?) and use container_of().
>>>
>>>
>>> You know, I'm not there that this is automatically better.
>>
>> Why not?  One less unsafe cast.
>
> It turns out there are a lot of cases in QEMU where there isn't an 
> obvious data structure to piggy back on.

Examples?

In that case you can always make a structure with a void * and a 
IOHandler, and you get your opaque.  But I don't really see how it can 
happen, the void * always points to a structure (or is ignored if 
there's a static instance), and you can add that IOHandler to the structure.

>
>>> Why is there an IOHandler and an IOEventHandler argument for init?
>
> So wouldn't you set the IOEventHandler directly in the IOHandler as 
> part of initialization?  IOW:
>
> struct MyDevice {
>     IOHandler iohandler;
> };
>
> iohandler.func = my_io_function;
> io_handler_init(&iohandler, fd, event_mask);
>
> That's the paradigm I'm used to at least.

You mean instead of passing .func to io_handler_init()?  I slightly 
prefer my version (less chance to forget the assignment) but I'm okay 
with either.

-- 
error compiling committee.c: too many arguments to function