Patchwork [2/7] iohandlers: Introduce a new API

login
register
mail settings
Submitter Amit Shah
Date Feb. 22, 2011, 10:18 a.m.
Message ID <3d41a2ca603b8132e2496976e89fca09fc246683.1298369272.git.amit.shah@redhat.com>
Download mbox | patch
Permalink /patch/83951/
State New
Headers show

Comments

Amit Shah - Feb. 22, 2011, 10:18 a.m.
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 <amit.shah@redhat.com>
---
 iohandler.h |   28 +++++++++++
 vl.c        |  144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 172 insertions(+), 0 deletions(-)
 create mode 100644 iohandler.h
Alon Levy - Feb. 22, 2011, 12:39 p.m.
On Tue, Feb 22, 2011 at 03:48:31PM +0530, Amit Shah wrote:
> 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?

> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  iohandler.h |   28 +++++++++++
>  vl.c        |  144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 172 insertions(+), 0 deletions(-)
>  create mode 100644 iohandler.h
> 
> diff --git a/iohandler.h b/iohandler.h
> new file mode 100644
> index 0000000..6553835
> --- /dev/null
> +++ b/iohandler.h
> @@ -0,0 +1,28 @@
> +/*
> + * Declarations for iohandler functions
> + *
> + * Copyright Red Hat, Inc. 2011
> + * Copyright Amit Shah <amit.shah@redhat.com> 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 */
> diff --git a/vl.c b/vl.c
> index e248ec4..9dabee2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2,6 +2,8 @@
>   * QEMU System Emulator
>   *
>   * Copyright (c) 2003-2008 Fabrice Bellard
> + * Copyright (C) 2011 Red Hat, Inc.
> + * Copyright (C) 2011 Amit Shah, <amit.shah@redhat.com>
>   *
>   * 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));
> -- 
> 1.7.4
> 
>
Amit Shah - Feb. 22, 2011, 1:09 p.m.
On (Tue) 22 Feb 2011 [14:39:03], Alon Levy wrote:
> On Tue, Feb 22, 2011 at 03:48:31PM +0530, Amit Shah wrote:
> > 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?

Hm, this will actually be a bug when we get rid of the _can_read
callbacks.  So the switch will have to change to something else for
sure.

		Amit
Avi Kivity - Feb. 22, 2011, 1:28 p.m.
On 02/22/2011 12:18 PM, Amit Shah wrote:
> 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 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().

> +
> +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.
Amit Shah - Feb. 22, 2011, 2:39 p.m.
On (Tue) 22 Feb 2011 [15:28:02], Avi Kivity wrote:
> On 02/22/2011 12:18 PM, Amit Shah wrote:
> >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 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.

IOHandler is taken.  I plan to switch back to IOHandler when the older
one is removed.

> Drop the opaque, instead put the IOHandler in there (or maybe the
> CharDev?) and use container_of().

Can put the IOHandler there; the chardev is just one user of
iohandlers, and there are others.

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

Thanks!

		Amit
Anthony Liguori - Feb. 22, 2011, 2:49 p.m.
On 02/22/2011 04:18 AM, Amit Shah wrote:
> 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<amit.shah@redhat.com>
> ---
>   iohandler.h |   28 +++++++++++
>   vl.c        |  144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 172 insertions(+), 0 deletions(-)
>   create mode 100644 iohandler.h
>
> diff --git a/iohandler.h b/iohandler.h
> new file mode 100644
> index 0000000..6553835
> --- /dev/null
> +++ b/iohandler.h
> @@ -0,0 +1,28 @@
> +/*
> + * Declarations for iohandler functions
> + *
> + * Copyright Red Hat, Inc. 2011
> + * Copyright Amit Shah<amit.shah@redhat.com>  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);
>    

As I previously suggested, we should return a connection handle instead 
of using an fd as an implicit handle.

Regards,

Anthony Liguori

> +
> +
> +#endif /* _QEMU_IOHANDLER_H */
> diff --git a/vl.c b/vl.c
> index e248ec4..9dabee2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2,6 +2,8 @@
>    * QEMU System Emulator
>    *
>    * Copyright (c) 2003-2008 Fabrice Bellard
> + * Copyright (C) 2011 Red Hat, Inc.
> + * Copyright (C) 2011 Amit Shah,<amit.shah@redhat.com>
>    *
>    * 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));
>
Anthony Liguori - Feb. 22, 2011, 3 p.m.
On 02/22/2011 07:28 AM, Avi Kivity wrote:
> On 02/22/2011 12:18 PM, Amit Shah wrote:
>> 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 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().

You know, I'm not there that this is automatically better.

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

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?

Regards,

Anthony Liguori
Avi Kivity - Feb. 22, 2011, 4:21 p.m.
On 02/22/2011 05:00 PM, Anthony Liguori wrote:
>>
>> 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.

>
>>> +
>>> +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.
>
> I don't think there should be a setter/getter for the event mask.  
> Just delete the handler and re-add it.
>

It's more epoll() compatible to update it.  I'm fine with reregistering 
though.

> 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.
Anthony Liguori - Feb. 22, 2011, 6:21 p.m.
On 02/22/2011 10:21 AM, Avi Kivity wrote:
> On 02/22/2011 05:00 PM, Anthony Liguori wrote:
>>>
>>> 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.

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

Regards,

Anthony Liguori

> 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.
>
Avi Kivity - Feb. 23, 2011, 12:40 p.m.
On 02/22/2011 08:21 PM, Anthony Liguori wrote:
> On 02/22/2011 10:21 AM, Avi Kivity wrote:
>> On 02/22/2011 05:00 PM, Anthony Liguori wrote:
>>>>
>>>> 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.

Examples?

In that case you can always make a structure with a void * and a 
IOHandler, and you get your opaque.  But I don't really see how it can 
happen, the void * always points to a structure (or is ignored if 
there's a static instance), and you can add that IOHandler to the structure.

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

You mean instead of passing .func to io_handler_init()?  I slightly 
prefer my version (less chance to forget the assignment) but I'm okay 
with either.

Patch

diff --git a/iohandler.h b/iohandler.h
new file mode 100644
index 0000000..6553835
--- /dev/null
+++ b/iohandler.h
@@ -0,0 +1,28 @@ 
+/*
+ * Declarations for iohandler functions
+ *
+ * Copyright Red Hat, Inc. 2011
+ * Copyright Amit Shah <amit.shah@redhat.com> 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 */
diff --git a/vl.c b/vl.c
index e248ec4..9dabee2 100644
--- a/vl.c
+++ b/vl.c
@@ -2,6 +2,8 @@ 
  * QEMU System Emulator
  *
  * Copyright (c) 2003-2008 Fabrice Bellard
+ * Copyright (C) 2011 Red Hat, Inc.
+ * Copyright (C) 2011 Amit Shah, <amit.shah@redhat.com>
  *
  * 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));