iohandlers: Add support for enabling/disabling individual handlers

PATCH Superseded Amit Shah 2 participants 16 messages Jan 13, 2011

lore

Share & tools

lore

Amit Shah

author

2011-01-13 13:00 UTC #0 lore raw

cover iohandlers: Add support for enabling/disabling individual handlers

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(-)

Signature · 1 line

1.7.3.4

Amit Shah

author

2011-01-13 13:00 UTC #1 lore raw

PATCH 1/5 iohandlers: Avoid code duplication

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

Signed-off-by Amit Shah <[email protected]>

---
 vl.c |   44 ++++++++++++++++++++++++++------------------
 1 files changed, 26 insertions(+), 18 deletions(-)

vl.c +26 ~~-18~~

index 0292184..9e365f6 100644
@@ -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;
 }

Signature · 1 line

1.7.3.4

Amit Shah

author

2011-01-13 13:00 UTC #2 lore raw

PATCH 2/5 iohandlers: Introduce assign_fd_handlers() and remove_fd_handlers

In reply to 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 <[email protected]>

---
 qemu-char.h |    6 ++++++
 vl.c        |   24 +++++++++++++++++++-----
 2 files changed, 25 insertions(+), 5 deletions(-)

qemu-char.h +6 -0

index e6ee6c4..0ef83f4 100644
@@ -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,

vl.c +19 -5

index 9e365f6..38e0a3c 100644
@@ -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,

Signature · 1 line

1.7.3.4

Gerd Hoffmann

2011-01-13 13:59 UTC #3 lore raw

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

Quoted 20 lines · 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<[email protected]>
---
  qemu-char.h |    6 ++++++
  vl.c        |   24 +++++++++++++++++++-----
  2 files changed, 25 insertions(+), 5 deletions(-)
qemu-char.h +0 -0
index e6ee6c4..0ef83f4 100644

qemu-char.h size_t qemu_chr_mem_osize(const CharDriverState *chr); · line 117

Quoted 9 lines

qemu-char.h +6 -0

@@ -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

Amit Shah

author

2011-01-13 14:10 UTC #4 lore raw

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

Quoted 8 lines · Gerd Hoffmann

qemu-char.h size_t qemu_chr_mem_osize(const CharDriverState *chr); · line 117

Quoted 6 lines

qemu-char.h +6 -0

+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

Amit Shah

author

2011-01-13 13:00 UTC #5 lore raw

PATCH 3/5 iohandlers: Allow each iohandler to be enabled/disabled individually

In reply to Amit Shah

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

Signed-off-by Amit Shah <[email protected]>

---
 qemu-char.h |    4 ++++
 vl.c        |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 58 insertions(+), 4 deletions(-)

qemu-char.h +4 -0

index 0ef83f4..e88a108 100644
@@ -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,

vl.c +54 -4

93 lines not on this page · raw message

Signature · 1 line

1.7.3.4

Gerd Hoffmann

2011-01-13 13:55 UTC #6 lore raw

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

vl.c void remove_fd_handlers(int fd) · line 1131

Quoted 8 lines · Amit Shah

vl.c +5 -1

-    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

Amit Shah

author

2011-01-13 14:01 UTC #7 lore raw

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

Quoted 10 lines · Gerd Hoffmann

vl.c void remove_fd_handlers(int fd) · line 1131

Quoted 8 lines · Amit Shah

{

vl.c +5 -1

-    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;
 }

Quoted 4 lines

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

Gerd Hoffmann

2011-01-13 14:09 UTC #8 lore raw

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

Quoted 11 lines · Amit Shah

Quoted 10 lines · Gerd Hoffmann

Quoted 8 lines · Amit Shah

vl.c +5 -1

-    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;
  }

Quoted 7 lines

Quoted 4 lines
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

Amit Shah

author

2011-01-13 14:18 UTC #9 lore raw

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

Quoted 17 lines · Gerd Hoffmann

Quoted 16 lines · Amit Shah
Quoted 14 lines · Gerd Hoffmann
vl.c void remove_fd_handlers(int fd) · line 1131
Quoted 8 lines · Amit Shah
{
vl.c +5 -1
-    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.

Quoted 7 lines

Quoted 2 lines
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

Gerd Hoffmann

2011-01-13 14:54 UTC #10 lore raw

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

Hi,

Quoted 6 lines

Quoted 4 lines
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.

Quoted 2 lines

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

Amit Shah

author

2011-01-13 13:00 UTC #11 lore raw

PATCH 4/5 iohandlers: Enable an iohandler only if the associated handler exists

In reply to 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 <[email protected]>

---
 vl.c |   29 ++++++++++++++++++++---------
 1 files changed, 20 insertions(+), 9 deletions(-)

vl.c +20 -9

index a0b14b5..42ec36a 100644
@@ -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);
             }

Signature · 1 line

1.7.3.4

Amit Shah

author

2011-01-13 13:00 UTC #12 lore raw

PATCH 5/5 iohandlers: Add IOHandlerOps struct

In reply to 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 <[email protected]>

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

qemu-char.h +2 -5

index e88a108..ebf9cd1 100644
@@ -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);

vl.c +32 ~~-19~~

127 lines not on this page · raw message

Signature · 1 line

1.7.3.4

Gerd Hoffmann

2011-01-13 14:02 UTC #13 lore raw

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

Hi,

vl.c int assign_fd_handlers(int fd, · line 1072

Quoted 3 lines

vl.c +3 -0

+    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

Amit Shah

author

2011-01-13 14:10 UTC #14 lore raw

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

Quoted 11 lines · Gerd Hoffmann

Hi,
vl.c int assign_fd_handlers(int fd, · line 1072
Quoted 3 lines
vl.c +3 -0
+    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

Gerd Hoffmann

2011-01-13 14:04 UTC #15 lore raw

In reply to Amit Shah

Quoted 15 lines · 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

Quoted 6 lines

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