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/