Patchwork [PATCHv2,1/2] char: separate device and system fd handlers

login
register
mail settings
Submitter Michael S. Tsirkin
Date Nov. 4, 2010, 6:06 p.m.
Message ID <c2325ff14d996bc98318a94636c569e165fc1971.1288892773.git.mst@redhat.com>
Download mbox | patch
Permalink /patch/70152/
State New
Headers show

Comments

Michael S. Tsirkin - Nov. 4, 2010, 6:06 p.m.
Create separate lists for system and device fd handlers.
Device handlers will not run while vm is stopped.
By default all fds are assumed system so they will
keep running as before.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 qemu-char.h |    6 +++-
 qemu-kvm.c  |    2 +-
 qemu-tool.c |   10 +++++
 vl.c        |  117 ++++++++++++++++++++++++++++++++++++++---------------------
 4 files changed, 92 insertions(+), 43 deletions(-)
Juan Quintela - Nov. 16, 2010, 10:24 a.m.
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> Create separate lists for system and device fd handlers.
> Device handlers will not run while vm is stopped.
> By default all fds are assumed system so they will
> keep running as before.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  qemu-char.h |    6 +++-
>  qemu-kvm.c  |    2 +-
>  qemu-tool.c |   10 +++++
>  vl.c        |  117 ++++++++++++++++++++++++++++++++++++++---------------------
>  4 files changed, 92 insertions(+), 43 deletions(-)
>
> diff --git a/qemu-char.h b/qemu-char.h
> index 18ad12b..ad09f56 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -101,7 +101,11 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd);
>  extern int term_escape_char;
>  
>  /* async I/O support */
> -
> +int qemu_set_fd_handler3(bool device, int fd,
> +                         IOCanReadHandler *fd_read_poll,
> +                         IOHandler *fd_read,
> +                         IOHandler *fd_write,
> +                         void *opaque);


No this horror.  Can't we just create:

qemu_set_device_handler()
qemu_set_system_handler()

or whatever named you like?

qemu_set_fd_handler2 is already bad enough, adding another one just make
things worse in my humble opinion.


> +int qemu_set_fd_handler3(bool device,
> +                         int fd,
>                           IOCanReadHandler *fd_read_poll,
>                           IOHandler *fd_read,
>                           IOHandler *fd_write,
>                           void *opaque)
>  {
> +    IOHandlerRecordList *list;
>      IOHandlerRecord *ioh;
>  
> +    list = device ? &device_io_handlers: &system_io_handlers;
> +

If you are going to use this, passing list paramenter instead of device
looks like a much better option.  It would indeed make things go better.

>      if (!fd_read && !fd_write) {
> -        QLIST_FOREACH(ioh, &io_handlers, next) {
> +        QLIST_FOREACH(ioh, list, next) {
>              if (ioh->fd == fd) {
>                  ioh->deleted = 1;
>                  break;
>              }
>          }
>      } else {
> -        QLIST_FOREACH(ioh, &io_handlers, next) {
> +        QLIST_FOREACH(ioh, list, next) {
>              if (ioh->fd == fd)
>                  goto found;
>          }
>          ioh = qemu_mallocz(sizeof(IOHandlerRecord));
> -        QLIST_INSERT_HEAD(&io_handlers, ioh, next);
> +        QLIST_INSERT_HEAD(list, ioh, next);
>      found:
>          ioh->fd = fd;
>          ioh->fd_read_poll = fd_read_poll;
> @@ -998,6 +1003,19 @@ int qemu_set_fd_handler2(int fd,
>      return 0;
>  }
>  
> +
> +/* 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)
> +{
> +    return qemu_set_fd_handler3(false, fd, fd_read_poll, fd_read, fd_write,
> +                                opaque);
> +}
> +
>  int qemu_set_fd_handler(int fd,
>                          IOHandler *fd_read,
>                          IOHandler *fd_write,
> @@ -1242,9 +1260,52 @@ void qemu_system_powerdown_request(void)
>      qemu_notify_event();
>  }
>  
> -void main_loop_wait(int nonblocking)
> +static inline int get_ioh_fds(IOHandlerRecordList *list,
> +                              int nfds, fd_set *rfds, fd_set *wfds)
>  {
>      IOHandlerRecord *ioh;
> +    QLIST_FOREACH(ioh, list, next) {
> +        if (ioh->deleted)
> +            continue;
> +        if (ioh->fd_read &&
> +            (!ioh->fd_read_poll ||
> +             ioh->fd_read_poll(ioh->opaque) != 0)) {
> +            FD_SET(ioh->fd, rfds);
> +            if (ioh->fd > nfds)
> +                nfds = ioh->fd;
> +        }
> +        if (ioh->fd_write) {
> +            FD_SET(ioh->fd, wfds);
> +            if (ioh->fd > nfds)
> +                nfds = ioh->fd;
> +        }
> +    }
> +    return nfds;
> +}
> +
> +static inline void call_ioh_fds(IOHandlerRecordList *list,
> +                                  fd_set *rfds, fd_set *wfds)
> +{
> +    IOHandlerRecord *ioh, *pioh;
> +
> +    QLIST_FOREACH_SAFE(ioh, list, next, pioh) {
> +        if (ioh->deleted) {
> +            QLIST_REMOVE(ioh, next);
> +            qemu_free(ioh);
> +            continue;
> +        }
> +        if (ioh->fd_read && FD_ISSET(ioh->fd, rfds)) {
> +            ioh->fd_read(ioh->opaque);
> +            if (!(ioh->fd_read_poll && ioh->fd_read_poll(ioh->opaque)))
> +                FD_CLR(ioh->fd, rfds);
> +        }
> +        if (ioh->fd_write && FD_ISSET(ioh->fd, wfds)) {
> +            ioh->fd_write(ioh->opaque);
> +        }
> +    }
> +}
> +void main_loop_wait(int nonblocking)
> +{
>      fd_set rfds, wfds, xfds;
>      int ret, nfds;
>      struct timeval tv;
> @@ -1260,26 +1321,13 @@ void main_loop_wait(int nonblocking)
>      os_host_main_loop_wait(&timeout);
>  
>      /* poll any events */
> -    /* XXX: separate device handlers from system ones */
>      nfds = -1;
>      FD_ZERO(&rfds);
>      FD_ZERO(&wfds);
>      FD_ZERO(&xfds);
> -    QLIST_FOREACH(ioh, &io_handlers, next) {
> -        if (ioh->deleted)
> -            continue;
> -        if (ioh->fd_read &&
> -            (!ioh->fd_read_poll ||
> -             ioh->fd_read_poll(ioh->opaque) != 0)) {
> -            FD_SET(ioh->fd, &rfds);
> -            if (ioh->fd > nfds)
> -                nfds = ioh->fd;
> -        }
> -        if (ioh->fd_write) {
> -            FD_SET(ioh->fd, &wfds);
> -            if (ioh->fd > nfds)
> -                nfds = ioh->fd;
> -        }
> +    nfds = get_ioh_fds(&system_io_handlers, nfds, &rfds, &wfds);
> +    if (vm_running) {
> +        nfds = get_ioh_fds(&device_io_handlers, nfds, &rfds, &wfds);
>      }
>  
>      tv.tv_sec = timeout / 1000;
> @@ -1291,22 +1339,9 @@ void main_loop_wait(int nonblocking)
>      ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
>      qemu_mutex_lock_iothread();
>      if (ret > 0) {
> -        IOHandlerRecord *pioh;
> -
> -        QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
> -            if (ioh->deleted) {
> -                QLIST_REMOVE(ioh, next);
> -                qemu_free(ioh);
> -                continue;
> -            }
> -            if (ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) {
> -                ioh->fd_read(ioh->opaque);
> -                if (!(ioh->fd_read_poll && ioh->fd_read_poll(ioh->opaque)))
> -                    FD_CLR(ioh->fd, &rfds);
> -            }
> -            if (ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) {
> -                ioh->fd_write(ioh->opaque);
> -            }
> +        call_ioh_fds(&system_io_handlers, &rfds, &wfds);
> +        if (vm_running) {
> +            call_ioh_fds(&device_io_handlers, &rfds, &wfds);
>          }
>      }

Patch

diff --git a/qemu-char.h b/qemu-char.h
index 18ad12b..ad09f56 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -101,7 +101,11 @@  CharDriverState *qemu_chr_open_eventfd(int eventfd);
 extern int term_escape_char;
 
 /* async I/O support */
-
+int qemu_set_fd_handler3(bool device, int fd,
+                         IOCanReadHandler *fd_read_poll,
+                         IOHandler *fd_read,
+                         IOHandler *fd_write,
+                         void *opaque);
 int qemu_set_fd_handler2(int fd,
                          IOCanReadHandler *fd_read_poll,
                          IOHandler *fd_read,
diff --git a/qemu-kvm.c b/qemu-kvm.c
index 625f286..56924a9 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -1621,7 +1621,7 @@  int kvm_main_loop(void)
 
     fcntl(sigfd, F_SETFL, O_NONBLOCK);
 
-    qemu_set_fd_handler2(sigfd, NULL, sigfd_handler, NULL,
+    qemu_set_fd_handler3(true, sigfd, NULL, sigfd_handler, NULL,
                          (void *)(unsigned long) sigfd);
 
     pthread_cond_broadcast(&qemu_system_cond);
diff --git a/qemu-tool.c b/qemu-tool.c
index b39af86..99d5938 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -111,6 +111,16 @@  int qemu_set_fd_handler2(int fd,
     return 0;
 }
 
+int qemu_set_fd_handler3(bool device,
+                         int fd,
+                         IOCanReadHandler *fd_read_poll,
+                         IOHandler *fd_read,
+                         IOHandler *fd_write,
+                         void *opaque)
+{
+    return 0;
+}
+
 int64_t qemu_get_clock(QEMUClock *clock)
 {
     qemu_timeval tv;
diff --git a/vl.c b/vl.c
index 42617c2..9e9e2a1 100644
--- a/vl.c
+++ b/vl.c
@@ -958,34 +958,39 @@  typedef struct IOHandlerRecord {
     QLIST_ENTRY(IOHandlerRecord) next;
 } IOHandlerRecord;
 
-static QLIST_HEAD(, IOHandlerRecord) io_handlers =
-    QLIST_HEAD_INITIALIZER(io_handlers);
+typedef QLIST_HEAD(, IOHandlerRecord) IOHandlerRecordList;
+static IOHandlerRecordList device_io_handlers =
+    QLIST_HEAD_INITIALIZER(device_io_handlers);
 
+static IOHandlerRecordList system_io_handlers =
+    QLIST_HEAD_INITIALIZER(system_io_handlers);
 
-/* 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,
+int qemu_set_fd_handler3(bool device,
+                         int fd,
                          IOCanReadHandler *fd_read_poll,
                          IOHandler *fd_read,
                          IOHandler *fd_write,
                          void *opaque)
 {
+    IOHandlerRecordList *list;
     IOHandlerRecord *ioh;
 
+    list = device ? &device_io_handlers: &system_io_handlers;
+
     if (!fd_read && !fd_write) {
-        QLIST_FOREACH(ioh, &io_handlers, next) {
+        QLIST_FOREACH(ioh, list, next) {
             if (ioh->fd == fd) {
                 ioh->deleted = 1;
                 break;
             }
         }
     } else {
-        QLIST_FOREACH(ioh, &io_handlers, next) {
+        QLIST_FOREACH(ioh, list, next) {
             if (ioh->fd == fd)
                 goto found;
         }
         ioh = qemu_mallocz(sizeof(IOHandlerRecord));
-        QLIST_INSERT_HEAD(&io_handlers, ioh, next);
+        QLIST_INSERT_HEAD(list, ioh, next);
     found:
         ioh->fd = fd;
         ioh->fd_read_poll = fd_read_poll;
@@ -998,6 +1003,19 @@  int qemu_set_fd_handler2(int fd,
     return 0;
 }
 
+
+/* 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)
+{
+    return qemu_set_fd_handler3(false, fd, fd_read_poll, fd_read, fd_write,
+                                opaque);
+}
+
 int qemu_set_fd_handler(int fd,
                         IOHandler *fd_read,
                         IOHandler *fd_write,
@@ -1242,9 +1260,52 @@  void qemu_system_powerdown_request(void)
     qemu_notify_event();
 }
 
-void main_loop_wait(int nonblocking)
+static inline int get_ioh_fds(IOHandlerRecordList *list,
+                              int nfds, fd_set *rfds, fd_set *wfds)
 {
     IOHandlerRecord *ioh;
+    QLIST_FOREACH(ioh, list, next) {
+        if (ioh->deleted)
+            continue;
+        if (ioh->fd_read &&
+            (!ioh->fd_read_poll ||
+             ioh->fd_read_poll(ioh->opaque) != 0)) {
+            FD_SET(ioh->fd, rfds);
+            if (ioh->fd > nfds)
+                nfds = ioh->fd;
+        }
+        if (ioh->fd_write) {
+            FD_SET(ioh->fd, wfds);
+            if (ioh->fd > nfds)
+                nfds = ioh->fd;
+        }
+    }
+    return nfds;
+}
+
+static inline void call_ioh_fds(IOHandlerRecordList *list,
+                                  fd_set *rfds, fd_set *wfds)
+{
+    IOHandlerRecord *ioh, *pioh;
+
+    QLIST_FOREACH_SAFE(ioh, list, next, pioh) {
+        if (ioh->deleted) {
+            QLIST_REMOVE(ioh, next);
+            qemu_free(ioh);
+            continue;
+        }
+        if (ioh->fd_read && FD_ISSET(ioh->fd, rfds)) {
+            ioh->fd_read(ioh->opaque);
+            if (!(ioh->fd_read_poll && ioh->fd_read_poll(ioh->opaque)))
+                FD_CLR(ioh->fd, rfds);
+        }
+        if (ioh->fd_write && FD_ISSET(ioh->fd, wfds)) {
+            ioh->fd_write(ioh->opaque);
+        }
+    }
+}
+void main_loop_wait(int nonblocking)
+{
     fd_set rfds, wfds, xfds;
     int ret, nfds;
     struct timeval tv;
@@ -1260,26 +1321,13 @@  void main_loop_wait(int nonblocking)
     os_host_main_loop_wait(&timeout);
 
     /* poll any events */
-    /* XXX: separate device handlers from system ones */
     nfds = -1;
     FD_ZERO(&rfds);
     FD_ZERO(&wfds);
     FD_ZERO(&xfds);
-    QLIST_FOREACH(ioh, &io_handlers, next) {
-        if (ioh->deleted)
-            continue;
-        if (ioh->fd_read &&
-            (!ioh->fd_read_poll ||
-             ioh->fd_read_poll(ioh->opaque) != 0)) {
-            FD_SET(ioh->fd, &rfds);
-            if (ioh->fd > nfds)
-                nfds = ioh->fd;
-        }
-        if (ioh->fd_write) {
-            FD_SET(ioh->fd, &wfds);
-            if (ioh->fd > nfds)
-                nfds = ioh->fd;
-        }
+    nfds = get_ioh_fds(&system_io_handlers, nfds, &rfds, &wfds);
+    if (vm_running) {
+        nfds = get_ioh_fds(&device_io_handlers, nfds, &rfds, &wfds);
     }
 
     tv.tv_sec = timeout / 1000;
@@ -1291,22 +1339,9 @@  void main_loop_wait(int nonblocking)
     ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
     qemu_mutex_lock_iothread();
     if (ret > 0) {
-        IOHandlerRecord *pioh;
-
-        QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
-            if (ioh->deleted) {
-                QLIST_REMOVE(ioh, next);
-                qemu_free(ioh);
-                continue;
-            }
-            if (ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) {
-                ioh->fd_read(ioh->opaque);
-                if (!(ioh->fd_read_poll && ioh->fd_read_poll(ioh->opaque)))
-                    FD_CLR(ioh->fd, &rfds);
-            }
-            if (ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) {
-                ioh->fd_write(ioh->opaque);
-            }
+        call_ioh_fds(&system_io_handlers, &rfds, &wfds);
+        if (vm_running) {
+            call_ioh_fds(&device_io_handlers, &rfds, &wfds);
         }
     }