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

[Qemu-devel] [PATCH 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().

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        |  162 +++++++++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 136 insertions(+), 33 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH 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 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        |   24 +++++++++++++++++++-----
 2 files changed, 25 insertions(+), 5 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..38e0a3c 100644
--- a/vl.c
+++ b/vl.c
@@ -1036,11 +1036,11 @@ static IOHandlerRecord *get_iohandler(int fd)
 
 /* 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)
+int assign_fd_handlers(int fd,
+                       IOCanReadHandler *fd_read_poll,
+                       IOHandler *fd_read,
+                       IOHandler *fd_write,
+                       void *opaque)
 {
     IOHandlerRecord *ioh;
 
@@ -1066,6 +1066,20 @@ int qemu_set_fd_handler2(int fd,
     return 0;
 }
 
+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 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        |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 58 insertions(+), 4 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 38e0a3c..a0b14b5 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;
@@ -1062,6 +1065,7 @@ int assign_fd_handlers(int fd,
     ioh->fd_write = fd_write;
     ioh->opaque = opaque;
     ioh->deleted = 0;
+    ioh->read_poll_enabled = ioh->read_enabled = ioh->write_enabled = false;
 
     return 0;
 }
@@ -1071,13 +1075,59 @@ void remove_fd_handlers(int fd)
     assign_fd_handlers(fd, NULL, NULL, NULL, NULL);
 }
 
+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;
+}
+
+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;
+}
+
+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;
+}
+
 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);
+    assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
+    set_read_poll_fd_action(fd, true);
+    set_read_fd_action(fd, true);
+    set_write_fd_action(fd, true);
+    return 0;
 }
 
 int qemu_set_fd_handler(int fd,
@@ -1341,14 +1391,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 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 a0b14b5..42ec36a 100644
--- a/vl.c
+++ b/vl.c
@@ -1084,7 +1084,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;
 }
@@ -1098,7 +1102,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;
 }
@@ -1112,7 +1120,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;
 }
@@ -1391,14 +1403,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;
@@ -1417,10 +1428,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 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        |   51 ++++++++++++++++++++++++++++++++-------------------
 2 files changed, 34 insertions(+), 24 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 42ec36a..d2ef63f 100644
--- a/vl.c
+++ b/vl.c
@@ -1007,11 +1007,15 @@ 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;
+    IOHandlerOps ops;
     int deleted;
     void *opaque;
     bool read_poll_enabled;
@@ -1025,6 +1029,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;
@@ -1039,17 +1047,16 @@ static IOHandlerRecord *get_iohandler(int fd)
 
 /* 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;
     }
@@ -1060,9 +1067,9 @@ 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.fd_read_poll = ops->fd_read_poll;
+    ioh->ops.fd_read = ops->fd_read;
+    ioh->ops.fd_write = ops->fd_write;
     ioh->opaque = opaque;
     ioh->deleted = 0;
     ioh->read_poll_enabled = ioh->read_enabled = ioh->write_enabled = false;
@@ -1072,7 +1079,7 @@ 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);
 }
 
 int set_read_poll_fd_action(int fd, bool enable)
@@ -1086,7 +1093,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;
     }
 
@@ -1104,7 +1111,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;
     }
 
@@ -1122,7 +1129,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;
     }
 
@@ -1135,7 +1142,13 @@ int qemu_set_fd_handler2(int fd,
                          IOHandler *fd_write,
                          void *opaque)
 {
-    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;
+    assign_fd_handlers(fd, &ops, opaque);
+
     set_read_poll_fd_action(fd, true);
     set_read_fd_action(fd, true);
     set_write_fd_action(fd, true);
@@ -1404,7 +1417,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;
@@ -1429,10 +1442,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 3/5] iohandlers: Allow each iohandler to be enabled/disabled individually

[Gerd Hoffmann]

On 01/13/11 14:00, Amit Shah wrote:
>   {
> -    return assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
> +    assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
> +    set_read_poll_fd_action(fd, true);
> +    set_read_fd_action(fd, true);
> +    set_write_fd_action(fd, true);
> +    return 0;
>   }

I'd suggest to move the *action calls into assign_fd_handlers() so the 
handlers default to being enabled in all cases.  This should match what 
most users need and thus minimize the number of *_action calls needed.

cheers,
   Gerd

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

[Gerd Hoffmann]

On 01/13/11 14:00, Amit Shah wrote:
> 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        |   24 +++++++++++++++++++-----
>   2 files changed, 25 insertions(+), 5 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);

A comment documenting the new functions would be nice.

cheers,
   Gerd

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

[Amit Shah]

On (Thu) Jan 13 2011 [14:55:25], Gerd Hoffmann wrote:
> On 01/13/11 14:00, Amit Shah wrote:
> >  {
> >-    return assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
> >+    assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
> >+    set_read_poll_fd_action(fd, true);
> >+    set_read_fd_action(fd, true);
> >+    set_write_fd_action(fd, true);
> >+    return 0;
> >  }
> 
> I'd suggest to move the *action calls into assign_fd_handlers() so
> the handlers default to being enabled in all cases.  This should
> match what most users need and thus minimize the number of *_action
> calls needed.

What may happen with that is the fd may get select()-ed for an operation
that it didn't want to be put on the queue for.

		Amit

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

[Gerd Hoffmann]

   Hi,

> +    ioh->ops.fd_read_poll = ops->fd_read_poll;
> +    ioh->ops.fd_read = ops->fd_read;
> +    ioh->ops.fd_write = ops->fd_write;

You can write this as "ioh->ops = *ops" btw.

I guess the long-term plan (to be committed after killing the last user 
of the old interface) is to store a pointer to the ops struct instead of 
copying it?

cheers,
   Gerd

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

[Gerd Hoffmann]

On 01/13/11 14:00, 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.
>
> 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 overall, just some minor nits, see replies to individual patches.

cheers,
   Gerd

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

[Gerd Hoffmann]

On 01/13/11 15:00, Amit Shah wrote:
> On (Thu) Jan 13 2011 [14:55:25], Gerd Hoffmann wrote:
>> On 01/13/11 14:00, Amit Shah wrote:
>>>   {
>>> -    return assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
>>> +    assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
>>> +    set_read_poll_fd_action(fd, true);
>>> +    set_read_fd_action(fd, true);
>>> +    set_write_fd_action(fd, true);
>>> +    return 0;
>>>   }
>>
>> I'd suggest to move the *action calls into assign_fd_handlers() so
>> the handlers default to being enabled in all cases.  This should
>> match what most users need and thus minimize the number of *_action
>> calls needed.
>
> What may happen with that is the fd may get select()-ed for an operation
> that it didn't want to be put on the queue for.

I can't see such a race window given that most qemu code runs serialized 
anyway.  If you call assign_fd_handlers() + set_write_fd_action(false) 
in sequence I can't see how a select call can happen inbetween ...

cheers,
   Gerd

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

[Amit Shah]

On (Thu) Jan 13 2011 [15:02:37], Gerd Hoffmann wrote:
>   Hi,
> 
> >+    ioh->ops.fd_read_poll = ops->fd_read_poll;
> >+    ioh->ops.fd_read = ops->fd_read;
> >+    ioh->ops.fd_write = ops->fd_write;
> 
> You can write this as "ioh->ops = *ops" btw.
> 
> I guess the long-term plan (to be committed after killing the last
> user of the old interface) is to store a pointer to the ops struct
> instead of copying it?

Yes, that's what I have in mind.

		Amit

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

[Amit Shah]

On (Thu) Jan 13 2011 [14:59:26], Gerd Hoffmann wrote:
> >+int assign_fd_handlers(int fd,
> >+                       IOCanReadHandler *fd_read_poll,
> >+                       IOHandler *fd_read,
> >+                       IOHandler *fd_write,
> >+                       void *opaque);
> >+void remove_fd_handlers(int fd);
> 
> A comment documenting the new functions would be nice.

Good idea; I'll do that.

		Amit

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

[Amit Shah]

On (Thu) Jan 13 2011 [15:08:51], Gerd Hoffmann wrote:
> On 01/13/11 15:00, Amit Shah wrote:
> >On (Thu) Jan 13 2011 [14:55:25], Gerd Hoffmann wrote:
> >>On 01/13/11 14:00, Amit Shah wrote:
> >>>  {
> >>>-    return assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
> >>>+    assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
> >>>+    set_read_poll_fd_action(fd, true);
> >>>+    set_read_fd_action(fd, true);
> >>>+    set_write_fd_action(fd, true);
> >>>+    return 0;
> >>>  }
> >>
> >>I'd suggest to move the *action calls into assign_fd_handlers() so
> >>the handlers default to being enabled in all cases.  This should
> >>match what most users need and thus minimize the number of *_action
> >>calls needed.
> >
> >What may happen with that is the fd may get select()-ed for an operation
> >that it didn't want to be put on the queue for.
> 
> I can't see such a race window given that most qemu code runs
> serialized anyway.  If you call assign_fd_handlers() +
> set_write_fd_action(false) in sequence I can't see how a select call
> can happen inbetween ...

Not today, but later when we have threads doing this stuff?  Should I
just leave a comment to take care of this for later?

		Amit

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

[Gerd Hoffmann]

   Hi,

>> I can't see such a race window given that most qemu code runs
>> serialized anyway.  If you call assign_fd_handlers() +
>> set_write_fd_action(false) in sequence I can't see how a select call
>> can happen inbetween ...
>
> Not today, but later when we have threads doing this stuff?

Unlikely I think.  Seems we will go offload specific tasks to threads 
using threadlets (especially in the block layer), but I expect the main 
even loop will not be splitted into multiple threads.

>  Should I
> just leave a comment to take care of this for later?

Thats fine I guess.  Or maybe add arguments to assign_fd_handlers() with 
the initial state.

cheers,
   Gerd