[Qemu-devel] [PATCH v2 0/5] iohandlers: Add support for enabling/disabling individual handlers

[Qemu-devel] [PATCH v2 0/5] iohandlers: Add support for enabling/disabling individual handlers

[Amit Shah]

Hi,

This patchset adds new interfaces to work with iohandlers.  It adds:

int assign_fd_handlers(int fd, IOHandlerOps *ops, void *opaque)
   -- Specify io handlers for an fd
int remove_fd_handlers(int fd)
   -- Remove fd handlers for fd (mark ioh for deletion)
int set_read_poll_fd_action(int fd, bool enable)
   -- Enable or disable the fd_read_poll fd handler
int set_read_fd_action(int fd, bool enable)
   -- Enable or disable the fd_read fd handler
int set_write_fd_action(int fd, bool enable)
   -- Enable or disable the fd_read fd handler

A new struct, IOHandlerOps, is added, to collect all the ops together
instead of passing individual ones to functions.

The older function, qemu_set_fd_handler2(), is now a wrapper to
assign_fd_handlers()  and can be deprecated by converting the existing
usage to assign_fd_handlers().

v2: Address comments from Gerd:
- add comments to new interfaces
- enable all specified handlers by default in assign_fd_handlers()
- Add comments for TODO items on deprecation of older interfaces.

Please apply.

Amit Shah (5):
  iohandlers: Avoid code duplication
  iohandlers: Introduce assign_fd_handlers() and remove_fd_handlers
  iohandlers: Allow each iohandler to be enabled/disabled individually
  iohandlers: Enable an iohandler only if the associated handler exists
  iohandlers: Add IOHandlerOps struct

 qemu-char.h |    7 ++
 vl.c        |  197 ++++++++++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 170 insertions(+), 34 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH v2 1/5] iohandlers: Avoid code duplication

[Amit Shah]

Add a get_iohandler() function instead of looking up the ioh twice in
qemu_set_fd_handler2().

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

diff --git a/vl.c b/vl.c
index 0292184..9e365f6 100644
--- a/vl.c
+++ b/vl.c
@@ -1022,6 +1022,17 @@ typedef struct IOHandlerRecord {
 static QLIST_HEAD(, IOHandlerRecord) io_handlers =
     QLIST_HEAD_INITIALIZER(io_handlers);
 
+static IOHandlerRecord *get_iohandler(int fd)
+{
+    IOHandlerRecord *ioh;
+
+    QLIST_FOREACH(ioh, &io_handlers, next) {
+        if (ioh->fd == fd) {
+            return ioh;
+        }
+    }
+    return NULL;
+}
 
 /* XXX: fd_read_poll should be suppressed, but an API change is
    necessary in the character devices to suppress fd_can_read(). */
@@ -1033,28 +1044,25 @@ int qemu_set_fd_handler2(int fd,
 {
     IOHandlerRecord *ioh;
 
-    if (!fd_read && !fd_write) {
-        QLIST_FOREACH(ioh, &io_handlers, next) {
-            if (ioh->fd == fd) {
-                ioh->deleted = 1;
-                break;
-            }
-        }
-    } else {
-        QLIST_FOREACH(ioh, &io_handlers, next) {
-            if (ioh->fd == fd)
-                goto found;
-        }
+    ioh = get_iohandler(fd);
+
+    if (ioh && !fd_read && !fd_write) {
+        ioh->deleted = 1;
+        return 0;
+    }
+
+    if (!ioh) {
         ioh = qemu_mallocz(sizeof(IOHandlerRecord));
         QLIST_INSERT_HEAD(&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->deleted = 0;
     }
+    ioh->fd_read_poll = fd_read_poll;
+    ioh->fd_read = fd_read;
+    ioh->fd_write = fd_write;
+    ioh->opaque = opaque;
+    ioh->deleted = 0;
+
     return 0;
 }
 
-- 
1.7.3.4

[Qemu-devel] [PATCH v2 2/5] iohandlers: Introduce assign_fd_handlers() and remove_fd_handlers

[Amit Shah]

This function will be used to assign fd handlers.  Future commits will
be enable each handler to be enabled/disabled individually.

Make qemu_set_fd_handler2() a wrapper to assign_fd_handlers().

remove_fd_handlers() removes all the assigned handlers and marks the
iohandler for deletion.  It's a wrapper to assign_fd_handlers() with
NULL handlers.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.h |    6 ++++++
 vl.c        |   36 +++++++++++++++++++++++++++++-------
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/qemu-char.h b/qemu-char.h
index e6ee6c4..0ef83f4 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -109,6 +109,12 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr);
 
 /* async I/O support */
 
+int assign_fd_handlers(int fd,
+                       IOCanReadHandler *fd_read_poll,
+                       IOHandler *fd_read,
+                       IOHandler *fd_write,
+                       void *opaque);
+void remove_fd_handlers(int fd);
 int qemu_set_fd_handler2(int fd,
                          IOCanReadHandler *fd_read_poll,
                          IOHandler *fd_read,
diff --git a/vl.c b/vl.c
index 9e365f6..30256e1 100644
--- a/vl.c
+++ b/vl.c
@@ -1034,13 +1034,17 @@ static IOHandlerRecord *get_iohandler(int fd)
     return NULL;
 }
 
-/* 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,
-                         IOCanReadHandler *fd_read_poll,
-                         IOHandler *fd_read,
-                         IOHandler *fd_write,
-                         void *opaque)
+/*
+ * Specify function pointers for the various IOHandlers for an fd here.
+ *
+ * XXX: fd_read_poll should be suppressed, but an API change is
+ * necessary in the character devices to suppress fd_can_read().
+ */
+int assign_fd_handlers(int fd,
+                       IOCanReadHandler *fd_read_poll,
+                       IOHandler *fd_read,
+                       IOHandler *fd_write,
+                       void *opaque)
 {
     IOHandlerRecord *ioh;
 
@@ -1066,6 +1070,24 @@ int qemu_set_fd_handler2(int fd,
     return 0;
 }
 
+/*
+ * Remove IO handlers for fd.  Marks the IOHandler, if one exists, for
+ * deletion.
+ */
+void remove_fd_handlers(int fd)
+{
+    assign_fd_handlers(fd, NULL, NULL, NULL, NULL);
+}
+
+int qemu_set_fd_handler2(int fd,
+                         IOCanReadHandler *fd_read_poll,
+                         IOHandler *fd_read,
+                         IOHandler *fd_write,
+                         void *opaque)
+{
+    return assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
+}
+
 int qemu_set_fd_handler(int fd,
                         IOHandler *fd_read,
                         IOHandler *fd_write,
-- 
1.7.3.4

[Qemu-devel] [PATCH v2 3/5] iohandlers: Allow each iohandler to be enabled/disabled individually

[Amit Shah]

Each iohandler for an fd can now be individually enabled or disabled.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.h |    4 +++
 vl.c        |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/qemu-char.h b/qemu-char.h
index 0ef83f4..e88a108 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -115,6 +115,10 @@ int assign_fd_handlers(int fd,
                        IOHandler *fd_write,
                        void *opaque);
 void remove_fd_handlers(int fd);
+int set_read_poll_fd_action(int fd, bool enable);
+int set_read_fd_action(int fd, bool enable);
+int set_write_fd_action(int fd, bool enable);
+
 int qemu_set_fd_handler2(int fd,
                          IOCanReadHandler *fd_read_poll,
                          IOHandler *fd_read,
diff --git a/vl.c b/vl.c
index 30256e1..b9c902a 100644
--- a/vl.c
+++ b/vl.c
@@ -1014,6 +1014,9 @@ typedef struct IOHandlerRecord {
     IOHandler *fd_write;
     int deleted;
     void *opaque;
+    bool read_poll_enabled;
+    bool read_enabled;
+    bool write_enabled;
     /* temporary data */
     struct pollfd *ufd;
     QLIST_ENTRY(IOHandlerRecord) next;
@@ -1035,6 +1038,57 @@ static IOHandlerRecord *get_iohandler(int fd)
 }
 
 /*
+ * Enable the fd_read_poll handler for fd if the fd_read_poll handler exists.
+ */
+int set_read_poll_fd_action(int fd, bool enable)
+{
+    IOHandlerRecord *ioh;
+
+    ioh = get_iohandler(fd);
+
+    if (!ioh) {
+        return -1;
+    }
+    ioh->read_poll_enabled = enable;
+
+    return 0;
+}
+
+/*
+ * Enable the fd_read handler for fd if the fd_read handler exists.
+ */
+int set_read_fd_action(int fd, bool enable)
+{
+    IOHandlerRecord *ioh;
+
+    ioh = get_iohandler(fd);
+
+    if (!ioh) {
+        return -1;
+    }
+    ioh->read_enabled = enable;
+
+    return 0;
+}
+
+/*
+ * Enable the fd_write handler for fd if the fd_write handler exists.
+ */
+int set_write_fd_action(int fd, bool enable)
+{
+    IOHandlerRecord *ioh;
+
+    ioh = get_iohandler(fd);
+
+    if (!ioh) {
+        return -1;
+    }
+    ioh->write_enabled = enable;
+
+    return 0;
+}
+
+/*
  * Specify function pointers for the various IOHandlers for an fd here.
  *
  * XXX: fd_read_poll should be suppressed, but an API change is
@@ -1067,6 +1121,20 @@ int assign_fd_handlers(int fd,
     ioh->opaque = opaque;
     ioh->deleted = 0;
 
+    /*
+     * To maintain compatibility with the callers that expect all
+     * handlers to be enabled.  Callers that wish for some handlers
+     * not to be enabled right away should ensure they call
+     * set_xx_fd_action(fd, false) after this call.
+     *
+     * Note: if gets threading and one day is capable of running
+     * main_loop_wait() in parallel to this function, ensure this call
+     * and any later set_xx_fd_action() calls can execute atomically.
+     */
+    set_read_poll_fd_action(fd, true);
+    set_read_fd_action(fd, true);
+    set_write_fd_action(fd, true);
+
     return 0;
 }
 
@@ -1079,6 +1147,10 @@ void remove_fd_handlers(int fd)
     assign_fd_handlers(fd, NULL, NULL, NULL, NULL);
 }
 
+/*
+ * TODO: Deprecate these two function calls in favour of
+ * assign_fd_handlers().
+ */
 int qemu_set_fd_handler2(int fd,
                          IOCanReadHandler *fd_read_poll,
                          IOHandler *fd_read,
@@ -1349,14 +1421,14 @@ void main_loop_wait(int nonblocking)
     QLIST_FOREACH(ioh, &io_handlers, next) {
         if (ioh->deleted)
             continue;
-        if (ioh->fd_read &&
+        if (ioh->fd_read && ioh->read_enabled &&
             (!ioh->fd_read_poll ||
-             ioh->fd_read_poll(ioh->opaque) != 0)) {
+             (!ioh->read_poll_enabled || ioh->fd_read_poll(ioh->opaque) != 0))) {
             FD_SET(ioh->fd, &rfds);
             if (ioh->fd > nfds)
                 nfds = ioh->fd;
         }
-        if (ioh->fd_write) {
+        if (ioh->fd_write && ioh->write_enabled) {
             FD_SET(ioh->fd, &wfds);
             if (ioh->fd > nfds)
                 nfds = ioh->fd;
-- 
1.7.3.4

[Qemu-devel] [PATCH v2 4/5] iohandlers: Enable an iohandler only if the associated handler exists

[Amit Shah]

If an iohandler is asked to be enabled but the handler doesn't exist,
don't enable the handler.

This can be used to simplify the conditions in main_loop_wait().

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

diff --git a/vl.c b/vl.c
index b9c902a..4aa4158 100644
--- a/vl.c
+++ b/vl.c
@@ -1049,7 +1049,11 @@ int set_read_poll_fd_action(int fd, bool enable)
     if (!ioh) {
         return -1;
     }
-    ioh->read_poll_enabled = enable;
+
+    ioh->read_poll_enabled = false;
+    if (enable && ioh->fd_read_poll) {
+        ioh->read_poll_enabled = true;
+    }
 
     return 0;
 }
@@ -1066,7 +1070,11 @@ int set_read_fd_action(int fd, bool enable)
     if (!ioh) {
         return -1;
     }
-    ioh->read_enabled = enable;
+
+    ioh->read_enabled = false;
+    if (enable && ioh->fd_read) {
+        ioh->read_enabled = true;
+    }
 
     return 0;
 }
@@ -1083,7 +1091,11 @@ int set_write_fd_action(int fd, bool enable)
     if (!ioh) {
         return -1;
     }
-    ioh->write_enabled = enable;
+
+    ioh->write_enabled = false;
+    if (enable && ioh->fd_write) {
+        ioh->write_enabled = true;
+    }
 
     return 0;
 }
@@ -1421,14 +1433,13 @@ void main_loop_wait(int nonblocking)
     QLIST_FOREACH(ioh, &io_handlers, next) {
         if (ioh->deleted)
             continue;
-        if (ioh->fd_read && ioh->read_enabled &&
-            (!ioh->fd_read_poll ||
-             (!ioh->read_poll_enabled || ioh->fd_read_poll(ioh->opaque) != 0))) {
+        if (ioh->read_enabled &&
+            (!ioh->read_poll_enabled || ioh->fd_read_poll(ioh->opaque) != 0)) {
             FD_SET(ioh->fd, &rfds);
             if (ioh->fd > nfds)
                 nfds = ioh->fd;
         }
-        if (ioh->fd_write && ioh->write_enabled) {
+        if (ioh->write_enabled) {
             FD_SET(ioh->fd, &wfds);
             if (ioh->fd > nfds)
                 nfds = ioh->fd;
@@ -1447,10 +1458,10 @@ void main_loop_wait(int nonblocking)
         IOHandlerRecord *pioh;
 
         QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
-            if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) {
+            if (!ioh->deleted && ioh->read_enabled && FD_ISSET(ioh->fd, &rfds)) {
                 ioh->fd_read(ioh->opaque);
             }
-            if (!ioh->deleted && ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) {
+            if (!ioh->deleted && ioh->write_enabled && FD_ISSET(ioh->fd, &wfds)) {
                 ioh->fd_write(ioh->opaque);
             }
 
-- 
1.7.3.4

[Qemu-devel] [PATCH v2 5/5] iohandlers: Add IOHandlerOps struct

[Amit Shah]

Collect all the handlers in a IOHandlerOps struct instead of being
passed one at a time to each function.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.h |    7 ++-----
 vl.c        |   56 ++++++++++++++++++++++++++++++++++++--------------------
 2 files changed, 38 insertions(+), 25 deletions(-)

diff --git a/qemu-char.h b/qemu-char.h
index e88a108..ebf9cd1 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -108,12 +108,9 @@ QString *qemu_chr_mem_to_qs(CharDriverState *chr);
 size_t qemu_chr_mem_osize(const CharDriverState *chr);
 
 /* async I/O support */
+typedef struct IOHandlerOps IOHandlerOps;
 
-int assign_fd_handlers(int fd,
-                       IOCanReadHandler *fd_read_poll,
-                       IOHandler *fd_read,
-                       IOHandler *fd_write,
-                       void *opaque);
+int assign_fd_handlers(int fd, const IOHandlerOps *ops, void *opaque);
 void remove_fd_handlers(int fd);
 int set_read_poll_fd_action(int fd, bool enable);
 int set_read_fd_action(int fd, bool enable);
diff --git a/vl.c b/vl.c
index 4aa4158..a98256c 100644
--- a/vl.c
+++ b/vl.c
@@ -1007,11 +1007,20 @@ void pcmcia_info(Monitor *mon)
 /***********************************************************/
 /* I/O handling */
 
-typedef struct IOHandlerRecord {
-    int fd;
+struct IOHandlerOps {
     IOCanReadHandler *fd_read_poll;
     IOHandler *fd_read;
     IOHandler *fd_write;
+};
+
+typedef struct IOHandlerRecord {
+    int fd;
+    /*
+     * TODO: once the users of qemu_set_fd_handler*() functions have
+     * been removed, just store the ops pointer instead of copying
+     * over each element here.
+     */
+    IOHandlerOps ops;
     int deleted;
     void *opaque;
     bool read_poll_enabled;
@@ -1025,6 +1034,10 @@ typedef struct IOHandlerRecord {
 static QLIST_HEAD(, IOHandlerRecord) io_handlers =
     QLIST_HEAD_INITIALIZER(io_handlers);
 
+static const IOHandlerOps null_ioh_ops = {
+    /* All ops are NULL */
+};
+
 static IOHandlerRecord *get_iohandler(int fd)
 {
     IOHandlerRecord *ioh;
@@ -1051,7 +1064,7 @@ int set_read_poll_fd_action(int fd, bool enable)
     }
 
     ioh->read_poll_enabled = false;
-    if (enable && ioh->fd_read_poll) {
+    if (enable && ioh->ops.fd_read_poll) {
         ioh->read_poll_enabled = true;
     }
 
@@ -1072,7 +1085,7 @@ int set_read_fd_action(int fd, bool enable)
     }
 
     ioh->read_enabled = false;
-    if (enable && ioh->fd_read) {
+    if (enable && ioh->ops.fd_read) {
         ioh->read_enabled = true;
     }
 
@@ -1093,7 +1106,7 @@ int set_write_fd_action(int fd, bool enable)
     }
 
     ioh->write_enabled = false;
-    if (enable && ioh->fd_write) {
+    if (enable && ioh->ops.fd_write) {
         ioh->write_enabled = true;
     }
 
@@ -1106,17 +1119,16 @@ int set_write_fd_action(int fd, bool enable)
  * XXX: fd_read_poll should be suppressed, but an API change is
  * necessary in the character devices to suppress fd_can_read().
  */
-int assign_fd_handlers(int fd,
-                       IOCanReadHandler *fd_read_poll,
-                       IOHandler *fd_read,
-                       IOHandler *fd_write,
-                       void *opaque)
+int assign_fd_handlers(int fd, const IOHandlerOps *ops, void *opaque)
 {
     IOHandlerRecord *ioh;
 
     ioh = get_iohandler(fd);
 
-    if (ioh && !fd_read && !fd_write) {
+    if (!ops) {
+        ops = &null_ioh_ops;
+    }
+    if (ioh && !ops->fd_read && !ops->fd_write) {
         ioh->deleted = 1;
         return 0;
     }
@@ -1127,9 +1139,7 @@ int assign_fd_handlers(int fd,
 
         ioh->fd = fd;
     }
-    ioh->fd_read_poll = fd_read_poll;
-    ioh->fd_read = fd_read;
-    ioh->fd_write = fd_write;
+    ioh->ops = *ops;
     ioh->opaque = opaque;
     ioh->deleted = 0;
 
@@ -1156,12 +1166,13 @@ int assign_fd_handlers(int fd,
  */
 void remove_fd_handlers(int fd)
 {
-    assign_fd_handlers(fd, NULL, NULL, NULL, NULL);
+    assign_fd_handlers(fd, NULL, NULL);
 }
 
 /*
  * TODO: Deprecate these two function calls in favour of
- * assign_fd_handlers().
+ * assign_fd_handlers().  When that happens, also see the TODO in the
+ * IOHandlerRecord struct above.
  */
 int qemu_set_fd_handler2(int fd,
                          IOCanReadHandler *fd_read_poll,
@@ -1169,7 +1180,12 @@ int qemu_set_fd_handler2(int fd,
                          IOHandler *fd_write,
                          void *opaque)
 {
-    return assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
+    IOHandlerOps ops;
+
+    ops.fd_read_poll = fd_read_poll;
+    ops.fd_read = fd_read;
+    ops.fd_write = fd_write;
+    return assign_fd_handlers(fd, &ops, opaque);
 }
 
 int qemu_set_fd_handler(int fd,
@@ -1434,7 +1450,7 @@ void main_loop_wait(int nonblocking)
         if (ioh->deleted)
             continue;
         if (ioh->read_enabled &&
-            (!ioh->read_poll_enabled || ioh->fd_read_poll(ioh->opaque) != 0)) {
+            (!ioh->read_poll_enabled || ioh->ops.fd_read_poll(ioh->opaque) != 0)) {
             FD_SET(ioh->fd, &rfds);
             if (ioh->fd > nfds)
                 nfds = ioh->fd;
@@ -1459,10 +1475,10 @@ void main_loop_wait(int nonblocking)
 
         QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
             if (!ioh->deleted && ioh->read_enabled && FD_ISSET(ioh->fd, &rfds)) {
-                ioh->fd_read(ioh->opaque);
+                ioh->ops.fd_read(ioh->opaque);
             }
             if (!ioh->deleted && ioh->write_enabled && FD_ISSET(ioh->fd, &wfds)) {
-                ioh->fd_write(ioh->opaque);
+                ioh->ops.fd_write(ioh->opaque);
             }
 
             /* Do this last in case read/write handlers marked it for deletion */
-- 
1.7.3.4

[Qemu-devel] Re: [PATCH v2 0/5] iohandlers: Add support for enabling/disabling individual handlers

[Gerd Hoffmann]

On 01/13/11 16:00, Amit Shah wrote:
> This patchset adds new interfaces to work with iohandlers.  It adds:
>
> int assign_fd_handlers(int fd, IOHandlerOps *ops, void *opaque)
>     -- Specify io handlers for an fd
> int remove_fd_handlers(int fd)
>     -- Remove fd handlers for fd (mark ioh for deletion)
> int set_read_poll_fd_action(int fd, bool enable)
>     -- Enable or disable the fd_read_poll fd handler
> int set_read_fd_action(int fd, bool enable)
>     -- Enable or disable the fd_read fd handler
> int set_write_fd_action(int fd, bool enable)
>     -- Enable or disable the fd_read fd handler
>
> A new struct, IOHandlerOps, is added, to collect all the ops together
> instead of passing individual ones to functions.
>
> The older function, qemu_set_fd_handler2(), is now a wrapper to
> assign_fd_handlers()  and can be deprecated by converting the existing
> usage to assign_fd_handlers().
>

Looks good now.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
   Gerd

[Qemu-devel] Re: [PATCH v2 0/5] iohandlers: Add support for enabling/disabling individual handlers

[Anthony Liguori]

On 01/13/2011 09:00 AM, Amit Shah wrote:
> Hi,
>
> This patchset adds new interfaces to work with iohandlers.  It adds:
>
> int assign_fd_handlers(int fd, IOHandlerOps *ops, void *opaque)
>     -- Specify io handlers for an fd
> int remove_fd_handlers(int fd)
>     -- Remove fd handlers for fd (mark ioh for deletion)
> int set_read_poll_fd_action(int fd, bool enable)
>     -- Enable or disable the fd_read_poll fd handler
> int set_read_fd_action(int fd, bool enable)
>     -- Enable or disable the fd_read fd handler
> int set_write_fd_action(int fd, bool enable)
>     -- Enable or disable the fd_read fd handler
>
> A new struct, IOHandlerOps, is added, to collect all the ops together
> instead of passing individual ones to functions.
>    

Instead of inventing new interfaces, I think we should steal^Wlearn from 
established interfaces.  Both libevent and glib have interfaces that 
essentially boil down to:

handle add_fd_event(loop, fd, ConditionMask, callback, opaque)
remove_event(loop, handle)

I think that's what we should move to.  All the stuff in our current 
loop around allowing suppressing of read events is terrible as it forces 
the main loop to poll.  That makes it impossible to use other main loops 
because it's completely unusual.

Regards,

Anthony Liguori

> The older function, qemu_set_fd_handler2(), is now a wrapper to
> assign_fd_handlers()  and can be deprecated by converting the existing
> usage to assign_fd_handlers().
>
> v2: Address comments from Gerd:
> - add comments to new interfaces
> - enable all specified handlers by default in assign_fd_handlers()
> - Add comments for TODO items on deprecation of older interfaces.
>
> Please apply.
>
> Amit Shah (5):
>    iohandlers: Avoid code duplication
>    iohandlers: Introduce assign_fd_handlers() and remove_fd_handlers
>    iohandlers: Allow each iohandler to be enabled/disabled individually
>    iohandlers: Enable an iohandler only if the associated handler exists
>    iohandlers: Add IOHandlerOps struct
>
>   qemu-char.h |    7 ++
>   vl.c        |  197 ++++++++++++++++++++++++++++++++++++++++++++++++----------
>   2 files changed, 170 insertions(+), 34 deletions(-)
>
>    

[Qemu-devel] Re: [PATCH v2 0/5] iohandlers: Add support for enabling/disabling individual handlers

[Amit Shah]

On (Thu) Jan 13 2011 [13:17:22], Anthony Liguori wrote:
> On 01/13/2011 09:00 AM, Amit Shah wrote:
> >Hi,
> >
> >This patchset adds new interfaces to work with iohandlers.  It adds:
> >
> >int assign_fd_handlers(int fd, IOHandlerOps *ops, void *opaque)
> >    -- Specify io handlers for an fd
> >int remove_fd_handlers(int fd)
> >    -- Remove fd handlers for fd (mark ioh for deletion)
> >int set_read_poll_fd_action(int fd, bool enable)
> >    -- Enable or disable the fd_read_poll fd handler
> >int set_read_fd_action(int fd, bool enable)
> >    -- Enable or disable the fd_read fd handler
> >int set_write_fd_action(int fd, bool enable)
> >    -- Enable or disable the fd_read fd handler
> >
> >A new struct, IOHandlerOps, is added, to collect all the ops together
> >instead of passing individual ones to functions.
> 
> Instead of inventing new interfaces, I think we should steal^Wlearn
> from established interfaces.

Agreed.

I do also think it'll be worthwhile pulling in one of the libraries to
reduce the amount of qemu-specific code we have in the other cases as
well.

>  Both libevent and glib have interfaces
> that essentially boil down to:
> 
> handle add_fd_event(loop, fd, ConditionMask, callback, opaque)
> remove_event(loop, handle)

This is quite similar to the Linux polling API.

I don't know what the 'loop' parameter would do, though.

> I think that's what we should move to.  All the stuff in our current
> loop around allowing suppressing of read events is terrible as it
> forces the main loop to poll.  That makes it impossible to use other
> main loops because it's completely unusual.

While setting new APIs possibly requires more discussion if we want to
do it once and right, since it's also an internal API, we can keep
evolving them.

So what's the accepted course of action now - take this new API, rework
it to look like that of some other library's and then rework the
main_loop, keep developing something in parallel till it satisfies
everyone, etc.?

Also -- this patchset was prompted by a bug in qemu chardevs that
freezes guests if they write faster than the chardevs can consume.
What should the strategy on fixing that bug be?

		Amit

[Qemu-devel] Re: [PATCH v2 0/5] iohandlers: Add support for enabling/disabling individual handlers

[Anthony Liguori]

On 01/17/2011 04:18 AM, Amit Shah wrote:
> On (Thu) Jan 13 2011 [13:17:22], Anthony Liguori wrote:
>    
>> On 01/13/2011 09:00 AM, Amit Shah wrote:
>>      
>>> Hi,
>>>
>>> This patchset adds new interfaces to work with iohandlers.  It adds:
>>>
>>> int assign_fd_handlers(int fd, IOHandlerOps *ops, void *opaque)
>>>     -- Specify io handlers for an fd
>>> int remove_fd_handlers(int fd)
>>>     -- Remove fd handlers for fd (mark ioh for deletion)
>>> int set_read_poll_fd_action(int fd, bool enable)
>>>     -- Enable or disable the fd_read_poll fd handler
>>> int set_read_fd_action(int fd, bool enable)
>>>     -- Enable or disable the fd_read fd handler
>>> int set_write_fd_action(int fd, bool enable)
>>>     -- Enable or disable the fd_read fd handler
>>>
>>> A new struct, IOHandlerOps, is added, to collect all the ops together
>>> instead of passing individual ones to functions.
>>>        
>> Instead of inventing new interfaces, I think we should steal^Wlearn
>> from established interfaces.
>>      
> Agreed.
>
> I do also think it'll be worthwhile pulling in one of the libraries to
> reduce the amount of qemu-specific code we have in the other cases as
> well.
>
>    
>>   Both libevent and glib have interfaces
>> that essentially boil down to:
>>
>> handle add_fd_event(loop, fd, ConditionMask, callback, opaque)
>> remove_event(loop, handle)
>>      
> This is quite similar to the Linux polling API.
>
> I don't know what the 'loop' parameter would do, though.
>    

It's to allow for multiple I/O threads.  There's one loop context per 
thread and you can register events specifically to a thread.

>> I think that's what we should move to.  All the stuff in our current
>> loop around allowing suppressing of read events is terrible as it
>> forces the main loop to poll.  That makes it impossible to use other
>> main loops because it's completely unusual.
>>      
> While setting new APIs possibly requires more discussion if we want to
> do it once and right, since it's also an internal API, we can keep
> evolving them.
>    

Yeah, but we should avoid unnecessary churn.  If we're going to do a 
major refactoring of an internal API, we should try to make the 
refactoring make sense as much as possible.

Eliminating the polling callbacks has been on the TODO for years now.  I 
don't think it make sense to refactor the API without at least doing this.

> So what's the accepted course of action now - take this new API, rework
> it to look like that of some other library's and then rework the
> main_loop, keep developing something in parallel till it satisfies
> everyone, etc.?
>
> Also -- this patchset was prompted by a bug in qemu chardevs that
> freezes guests if they write faster than the chardevs can consume.
> What should the strategy on fixing that bug be?
>    

Fix the loop API such that we're not just fixing one bug but that we can 
address a bunch of other bugs that are out there.

Regards,

Anthony Liguori

> 		Amit
>    

Re: [Qemu-devel] Re: [PATCH v2 0/5] iohandlers: Add support for enabling/disabling individual handlers

[Michael Roth]

On 01/17/2011 08:57 AM, Anthony Liguori wrote:
> On 01/17/2011 04:18 AM, Amit Shah wrote:
>> On (Thu) Jan 13 2011 [13:17:22], Anthony Liguori wrote:
>>> On 01/13/2011 09:00 AM, Amit Shah wrote:
>>>> Hi,
>>>>
>>>> This patchset adds new interfaces to work with iohandlers. It adds:
>>>>
>>>> int assign_fd_handlers(int fd, IOHandlerOps *ops, void *opaque)
>>>> -- Specify io handlers for an fd
>>>> int remove_fd_handlers(int fd)
>>>> -- Remove fd handlers for fd (mark ioh for deletion)
>>>> int set_read_poll_fd_action(int fd, bool enable)
>>>> -- Enable or disable the fd_read_poll fd handler
>>>> int set_read_fd_action(int fd, bool enable)
>>>> -- Enable or disable the fd_read fd handler
>>>> int set_write_fd_action(int fd, bool enable)
>>>> -- Enable or disable the fd_read fd handler
>>>>
>>>> A new struct, IOHandlerOps, is added, to collect all the ops together
>>>> instead of passing individual ones to functions.
>>> Instead of inventing new interfaces, I think we should steal^Wlearn
>>> from established interfaces.
>> Agreed.
>>
>> I do also think it'll be worthwhile pulling in one of the libraries to
>> reduce the amount of qemu-specific code we have in the other cases as
>> well.
>>
>>> Both libevent and glib have interfaces
>>> that essentially boil down to:
>>>
>>> handle add_fd_event(loop, fd, ConditionMask, callback, opaque)
>>> remove_event(loop, handle)
>> This is quite similar to the Linux polling API.
>>
>> I don't know what the 'loop' parameter would do, though.
>
> It's to allow for multiple I/O threads. There's one loop context per
> thread and you can register events specifically to a thread.
>

In case you're interested, to make the fd handler stuff accessible to 
qemu tools I have some code that moves these interfaces into backend 
functions in another file, qemu-ioh.c, where the IOHandlerRecord list is 
supplied by the caller to track state.

This would extend naturally to threads by just having them maintain 
their own IOHandlerRecord lists as well, and stuff that is meant to 
interact with vl.c:io_handlers can be done via wrapper functions for 
backward-compatibility.

If that seems like a reasonable approach maybe you could leverage these 
patches remove some of the scope for your proposed changes:

http://repo.or.cz/w/qemu/mdroth.git/patch/56a391e85124382ddcbdb64dcc05aadfc965f000

http://repo.or.cz/w/qemu/mdroth.git/patch/f4428e2cdf5da231bb30ce1ed9eb5ce567a9dc55

[Qemu-devel] Re: [PATCH v2 0/5] iohandlers: Add support for enabling/disabling individual handlers

[Amit Shah]

On (Mon) Jan 17 2011 [08:57:16], Anthony Liguori wrote:
> >Also -- this patchset was prompted by a bug in qemu chardevs that
> >freezes guests if they write faster than the chardevs can consume.
> >What should the strategy on fixing that bug be?
> 
> Fix the loop API such that we're not just fixing one bug but that we
> can address a bunch of other bugs that are out there.

But what's the right fix?

* Pull in glib or some other library
* Mimic API from some other library

		Amit

[Qemu-devel] Re: [PATCH v2 0/5] iohandlers: Add support for enabling/disabling individual handlers

[Anthony Liguori]

On 01/18/2011 05:56 AM, Amit Shah wrote:
> On (Mon) Jan 17 2011 [08:57:16], Anthony Liguori wrote:
>    
>>> Also -- this patchset was prompted by a bug in qemu chardevs that
>>> freezes guests if they write faster than the chardevs can consume.
>>> What should the strategy on fixing that bug be?
>>>        
>> Fix the loop API such that we're not just fixing one bug but that we
>> can address a bunch of other bugs that are out there.
>>      
> But what's the right fix?
>
> * Pull in glib or some other library
> * Mimic API from some other library
>    

1) get rid of poll callbacks

These kill us right now because it makes it impossible to use poll/epoll 
or to use some other API.

2) switch to a single callback and an event mask

We can't do this without (1) because the can_read callback takes a 
different signature.  But read/write callbacks can be a single callback 
(but it needs to be changed to accept an event mask).

3) allow the event mask to be changed via a new API

This gives the functionality you're looking for while making a huge 
improvement to the internal API.

A quick python/perl script can probably do 90% of the work here.

Regards,

Anthony Liguori

> 		Amit
>