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(a)redhat.com | libxml GNOME XML XSLT toolkit
http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine
http://rpmfind.net/