[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] PATCH 1/3: Split out simple event loop



On Sun, Jun 17, 2007 at 10:52:55PM +0100, Daniel P. Berrange wrote:
> The following patch adds a qemud/event.c & qemud/event.h file providing a
> general purpose event loop built around poll. Users register file handles
> and associated callbacks, and / or timers. The qemud.c file is changed to
> make use of these APIs for dealing with server, client, and VM file handles
> and/or sockets. This decouples much of the QEMU VM I/O code from the main
> qemud.c daemon code.
> 
>  qemud/event.c   |  311 +++++++++++++++++++++++++++++++++++++++++
>  qemud/event.h   |   13 +
>  qemud/Makefile.am |    3 
>  qemud/qemud.c     |  401 +++++++++++++++++++++++++++++-------------------------
>  qemud/remote.c    |    5 
>  5 files changed, 546 insertions(+), 187 deletions(-)

  As I understand taht patch could be applied alone without disturbing
operations, right ?

> diff -r 93de958458cb qemud/Makefile.am
> --- a/qemud/Makefile.am	Fri Jun 15 14:27:39 2007 +0000
> +++ b/qemud/Makefile.am	Sun Jun 17 16:26:41 2007 -0400
> @@ -16,7 +16,8 @@ libvirt_qemud_SOURCES = \
>  		buf.c buf.h \
>  		protocol.h protocol.c \
>  		remote_protocol.h remote_protocol.c \
> -		remote.c
> +		remote.c \
> +                event.c event.h
>  #-D_XOPEN_SOURCE=600 -D_XOPEN_SOURCE_EXTENDED=1 -D_POSIX_C_SOURCE=199506L
>  libvirt_qemud_CFLAGS = \
>          -I$(top_srcdir)/include -I$(top_builddir)/include $(LIBXML_CFLAGS) \
> diff -r 93de958458cb qemud/event.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/qemud/event.c	Sun Jun 17 16:26:34 2007 -0400
> @@ -0,0 +1,311 @@

  In general a severe lack of comment, I assume it's an early review version.

> +int virEventAddHandle(int fd, int events, virEventHandleCallback cb, void *opaque) {
> +    struct virEventHandle *tmp;
> +
> +    printf("Add handle %d %d %p %p\n", fd, events, cb, opaque);
> +    tmp = realloc(eventLoop.handles, sizeof(struct virEventHandle) * (eventLoop.nhandles+1));
> +    if (!tmp) {
> +        return -1;
> +    }

  Hum, realloc( , n+1) always leave me with a unpleasant feeling. I really
prefer a current usage size, an allocated size and growing by doubling the
block only when needed.

> +    eventLoop.handles = tmp;
> +
> +    eventLoop.handles[eventLoop.nhandles].fd = fd;
> +    eventLoop.handles[eventLoop.nhandles].events = events;
> +    eventLoop.handles[eventLoop.nhandles].cb = cb;
> +    eventLoop.handles[eventLoop.nhandles].opaque = opaque;
> +    eventLoop.handles[eventLoop.nhandles].deleted = 0;
> +
> +    eventLoop.nhandles++;
> +
> +    return 0;
> +}
> +
> +int virEventRemoveHandle(int fd) {
> +    int i;
> +    printf("Remove handle %d\n", fd);
> +    for (i = eventLoop.nhandles-1 ; i >= 0 ; i--) {
> +        if (eventLoop.handles[i].fd == fd) {
> +            printf("mark delete %d\n", i);
> +            eventLoop.handles[i].deleted = 1;
> +            return 0;
> +        }
> +    }
> +    return -1;
> +}
> +
> +
> +int virEventAddTimeout(int timeout, virEventTimeoutCallback cb, void *opaque) {
> +    struct virEventTimeout *tmp;
> +    struct timeval tv;
> +
> +    if (gettimeofday(&tv, NULL) < 0) {
> +        return -1;
> +    }
> +
> +    tmp = realloc(eventLoop.timeouts, sizeof(struct virEventTimeout) * (eventLoop.ntimeouts+1));
> +    if (!tmp) {
> +        return -1;
> +    }
> +    eventLoop.timeouts = tmp;
> +
> +    eventLoop.timeouts[eventLoop.ntimeouts].timer = nextTimer++;
> +    eventLoop.timeouts[eventLoop.ntimeouts].timeout = timeout;
> +    eventLoop.timeouts[eventLoop.ntimeouts].cb = cb;
> +    eventLoop.timeouts[eventLoop.ntimeouts].opaque = opaque;
> +    eventLoop.timeouts[eventLoop.ntimeouts].deleted = 0;
> +    eventLoop.timeouts[eventLoop.ntimeouts].expiresAt =
> +        (((unsigned long long)tv.tv_sec)*1000) +
> +        (((unsigned long long)tv.tv_usec)/1000);
> +
> +    eventLoop.ntimeouts++;
> +
> +    return 0;
> +}
> +
> +int virEventRemoveTimeout(int timer) {
> +    int i;
> +    for (i = eventLoop.ntimeouts-1 ; i >=0 ; i--) {
> +        if (eventLoop.timeouts[i].timer == timer) {
> +            eventLoop.timeouts[i].deleted = 1;
> +            return 0;
> +        }
> +    }
> +    return -1;
> +}
> +
> +
> +static int virEventCalculateTimeout(int *timeout) {
> +    unsigned long long then = 0;
> +    int i;
> +
> +    /* Figure out if we need a timeout */
> +    for (i = 0 ; i < eventLoop.ntimeouts ; i++) {
> +        if (then == 0 ||
> +            eventLoop.timeouts[i].expiresAt < then)
> +            then = eventLoop.timeouts[i].expiresAt;
> +    }
> +
> +    /* Calculate how long we should wait for a timeout if needed */
> +    if (then > 0) {
> +        struct timeval tv;
> +
> +        if (gettimeofday(&tv, NULL) < 0) {
> +            return -1;
> +        }
> +
> +        *timeout = then -
> +            ((((unsigned long long)tv.tv_sec)*1000) +
> +             (((unsigned long long)tv.tv_usec)/1000));
> +
> +        if (*timeout < 0)
> +            *timeout = 1;
> +    } else {
> +        *timeout = -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static struct pollfd *virEventMakePollFDs(void) {
> +    struct pollfd *fds;
> +    int i;
> +
> +    /* Setup the poll file handle data structs */
> +    if (!(fds = malloc(sizeof(struct pollfd)*eventLoop.nhandles)))
> +        return NULL;
> +
> +    for (i = 0 ; i < eventLoop.nhandles ; i++) {
> +        fds[i].fd = eventLoop.handles[i].fd;
> +        fds[i].events = eventLoop.handles[i].events;
> +        fds[i].revents = 0;
> +        printf("Wait for %d %d\n", eventLoop.handles[i].fd, eventLoop.handles[i].events);
> +    }
[...]
same here

> +
> +static int virEventCleanupTimeouts(void) {
> +    int i;
> +    for (i = 0 ; i < eventLoop.ntimeouts ; ) {
> +        struct virEventTimeout *tmp;
> +        if (!eventLoop.handles[i].deleted) {
> +            i++;
> +            continue;
> +        }
> +
> +        if ((i+1) < eventLoop.ntimeouts) {
> +            memmove(eventLoop.timeouts+i,
> +                    eventLoop.timeouts+i+1,
> +                    sizeof(struct virEventTimeout)*(eventLoop.ntimeouts-(i+1)));
> +        }
> +
> +        tmp = realloc(eventLoop.timeouts, sizeof(struct virEventTimeout) * (eventLoop.ntimeouts-1));
> +        if (!tmp) {
> +            return -1;
> +        }
> +        eventLoop.timeouts = tmp;
> +        eventLoop.ntimeouts--;
> +    }
> +    return 0;
> +}

  realloc( -1); in a loop, I would rather realloc at the end or just keep
a maximum allocated block and realloc to maxtimeouts / 2 only if
ntimeouts < (maxtimeouts / 2) -1)

> +static int virEventCleanupHandles(void) {
[...]
> +        tmp = realloc(eventLoop.handles, sizeof(struct virEventHandle) * (eventLoop.nhandles-1));
> +        if (!tmp) {
> +            return -1;
> +        }
> +        eventLoop.handles = tmp;
> +        eventLoop.nhandles--;
> +    }
> +    return 0;
> +}

  Same here

> +int virEventRunOnce(void) {
> +    struct pollfd *fds;
> +    int ret, timeout;
> +
> +    if (!(fds = virEventMakePollFDs()))
> +        return -1;
> +
> +    if (virEventCalculateTimeout(&timeout) < 0) {
> +        free(fds);
> +        return -1;
> +    }
> +
> + retry:
> +    printf("Poll on %d handles %p timeout %d\n", eventLoop.nhandles, fds, timeout);
> +    ret = poll(fds, eventLoop.nhandles, timeout);
> +    printf("Poll got %d event\n", ret);
> +    if (ret < 0) {
> +        if (errno == EINTR) {
> +            goto retry;
> +        }
> +        free(fds);
> +        return -1;
> +    }
> +    if (virEventDispatchTimeouts() < 0) {
> +        free(fds);
> +        return -1;
> +    }
> +
> +    if (ret > 0 &&
> +        virEventDispatchHandles(fds) < 0) {
> +        free(fds);
> +        return -1;
> +    }
> +    free(fds);
> +
> +    if (virEventCleanupTimeouts() < 0)
> +        return -1;
> +
> +    if (virEventCleanupHandles() < 0)
> +        return -1;
> +
> +    return 0;
> +}
...
> diff -r 93de958458cb qemud/event.h
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/qemud/event.h	Sun Jun 17 16:26:34 2007 -0400
> @@ -0,0 +1,13 @@
> +
> +
> +typedef void (*virEventHandleCallback)(int fd, int events, void *opaque);
> +
> +int virEventAddHandle(int fd, int events, virEventHandleCallback cb, void *opaque);
> +int virEventRemoveHandle(int fd);
> +
> +typedef void (*virEventTimeoutCallback)(int timer, void *opaque);
> +
> +int virEventAddTimeout(int timeout, virEventTimeoutCallback cb, void *opaque);
> +int virEventRemoveTimeout(int timer);
> +
> +int virEventRunOnce(void);
> diff -r 93de958458cb qemud/qemud.c
> --- a/qemud/qemud.c	Fri Jun 15 14:27:39 2007 +0000
> +++ b/qemud/qemud.c	Sun Jun 17 17:37:07 2007 -0400
> @@ -61,6 +61,7 @@
>  #include "driver.h"
>  #include "conf.h"
>  #include "iptables.h"
> +#include "event.h"
>  
>  static int godaemon = 0;        /* -d: Be a daemon */
>  static int verbose = 0;         /* -v: Verbose mode */
> @@ -109,6 +110,13 @@ static void sig_handler(int sig) {
>      }
>      errno = origerrno;
>  }
> +
> +static void qemudDispatchVMEvent(int fd, int events, void *opaque);
> +static void qemudDispatchClientEvent(int fd, int events, void *opaque);
> +static void qemudDispatchServerEvent(int fd, int events, void *opaque);
> +static int qemudRegisterClientEvent(struct qemud_server *server,
> +                                    struct qemud_client *client,
> +                                    int remove);
>  
>  static int
>  remoteInitializeGnuTLS (void)
> @@ -184,8 +192,10 @@ remoteInitializeGnuTLS (void)
>      return 0;
>  }
>  
> -static int qemudDispatchSignal(struct qemud_server *server)
> -{
> +static void qemudDispatchSignalEvent(int fd ATTRIBUTE_UNUSED,
> +                                     int events ATTRIBUTE_UNUSED,
> +                                     void *opaque) {
> +    struct qemud_server *server = (struct qemud_server *)opaque;
>      unsigned char sigc;
>      struct qemud_vm *vm;
>      struct qemud_network *network;
> @@ -194,7 +204,7 @@ static int qemudDispatchSignal(struct qe
>      if (read(server->sigread, &sigc, 1) != 1) {
>          qemudLog(QEMUD_ERR, "Failed to read from signal pipe: %s",
>                   strerror(errno));
> -        return -1;
> +        return;
>      }
>  
>      ret = 0;
> @@ -266,7 +276,8 @@ static int qemudDispatchSignal(struct qe
>          break;
>      }
>  
> -    return ret;
> +    if (ret != 0)
> +        server->shutdown = 1;
>  }

is asynchronous error handling really better there ?

>  static int qemudSetCloseExec(int fd) {
> @@ -474,19 +485,16 @@ static int qemudListenUnix(struct qemud_
>      }
[...]
> diff -r 93de958458cb qemud/remote.c
> --- a/qemud/remote.c	Fri Jun 15 14:27:39 2007 +0000
> +++ b/qemud/remote.c	Sun Jun 17 16:26:54 2007 -0400
> @@ -691,7 +691,7 @@ remoteDispatchDomainGetInfo (struct qemu
>          remoteDispatchError (client, req, "domain not found");
>          return -2;
>      }
> -
> +    printf("------------------------------------- %d %s\n", dom->id, dom->name);

  I assume those are debugging left over, right ?

>      if (virDomainGetInfo (dom, &info) == -1)
>          return -1;
>  
> @@ -862,7 +862,7 @@ remoteDispatchDomainLookupById (struct q
>  
>      dom = virDomainLookupByID (client->conn, args->id);
>      if (dom == NULL) return -1;
> -
> +    printf("******************Loopu %d %s\n", dom->id, dom->name);
>      make_nonnull_domain (&ret->dom, dom);
>  
>      return 0;
> @@ -1539,6 +1539,7 @@ get_nonnull_domain (virConnectPtr conn, 
>      /* Should we believe the domain.id sent by the client?  Maybe
>       * this should be a check rather than an assignment? XXX
>       */
> +    printf("********************* %d\n", domain.id);
>      if (dom) dom->id = domain.id;
>      return dom;
>  }

  My main concern is lack of comments and the allocation stategy,

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]