New iohandler API

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.

Quoted 5 lines · 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.

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.

Quoted 7 lines · Avi Kivity

Quoted 5 lines · 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.

Quoted 3 lines

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?

Quoted 12 lines · Amit Shah

Quoted 10 lines · Avi Kivity

Quoted 5 lines · 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.

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.

Quoted 3 lines

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.

Quoted 17 lines · Avi Kivity

Quoted 15 lines · Amit Shah

Quoted 10 lines · Avi Kivity

Quoted 5 lines · 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.

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

Quoted 3 lines

Does "deprecate" mean "completely remove"?

If so I agree.

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

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

---
 iohandler.h |   28 +++++++++++
 vl.c        |  144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 172 insertions(+), 0 deletions(-)
 create mode 100644 iohandler.h

iohandler.h new file +28 -0

new file mode 100644
index 0000000..6553835
@@ -0,0 +1,28 @@
+/*
+ * Declarations for iohandler functions
+ *
+ * Copyright Red Hat, Inc. 2011
+ * Copyright Amit Shah <[email protected]> 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 */

vl.c +144 -0

index e248ec4..9dabee2 100644
@@ -2,6 +2,8 @@
  * QEMU System Emulator
  *
  * Copyright (c) 2003-2008 Fabrice Bellard
+ * Copyright (C) 2011 Red Hat, Inc.
+ * Copyright (C) 2011 Amit Shah, <[email protected]>
  *
  * 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));

Signature · 1 line

1.7.4

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

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?

Quoted 6 lines · 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.

iohandler.h line 19

Quoted 7 lines

iohandler.h new file +7 -0

+
+/* 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().

iohandler.h line 26

Quoted 7 lines

iohandler.h new file +7 -0

+
+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.

iohandler.h line 19

Quoted 13 lines · 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.

iohandler.h new file +7 -0

+
+/* 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().

iohandler.h line 26

Quoted 7 lines

iohandler.h new file +7 -0

+
+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.

Quoted 2 lines

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.

Quoted 14 lines

iohandler.h line 26

Quoted 8 lines

iohandler.h new file +7 -0

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

Quoted 1 line

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?

Quoted 5 lines · Anthony Liguori

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 8 lines · Avi Kivity

Quoted 5 lines · Anthony Liguori

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

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.

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

---
  iohandler.h |   28 +++++++++++
  vl.c        |  144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 172 insertions(+), 0 deletions(-)

create mode 100644 iohandler.h

iohandler.h new file +15 -0

new file mode 100644
index 0000000..6553835
@@ -0,0 +1,28 @@
+/*
+ * Declarations for iohandler functions
+ *
+ * Copyright Red Hat, Inc. 2011
+ * Copyright Amit Shah<[email protected]>  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)

iohandler.h new file +3 -0

+
+
+#endif /* _QEMU_IOHANDLER_H */

vl.c +144 -0

index e248ec4..9dabee2 100644
@@ -2,6 +2,8 @@
   * QEMU System Emulator
   *
   * Copyright (c) 2003-2008 Fabrice Bellard
+ * Copyright (C) 2011 Red Hat, Inc.
+ * Copyright (C) 2011 Amit Shah,<[email protected]>
   *
   * 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));