[Qemu-devel] Re: [PATCHv2 1/2] char: separate device and system fd handlers

[Juan Quintela <quintela@redhat.com>]

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