Hi Dan, On Fri, 2007-03-02 at 14:35 +0000, Daniel P. Berrange wrote:
+static int qemudOpenMonitor(struct qemud_vm *vm, const char *monitor) {
#define MONITOR_POLL_TIMEOUT 5
+ int monfd; + char buffer[1024]; + int got = 0; + + if (!(monfd = open(monitor, O_RDWR))) { + return -1; + } + if (qemudSetCloseExec(monfd) < 0) + goto error; + if (qemudSetNonBlock(monfd) < 0) + goto error; + + /* Consume & discard the initial greeting */ + for(;;) { + int ret; + + ret = read(monfd, buffer+got, sizeof(buffer)-got-1);
What happens if the buffer fills up ?
+ if (ret == 0) + goto error; + if (ret < 0) { + struct pollfd fd = { .fd = monfd, .events = POLLIN }; + if (errno != EAGAIN && + errno != EINTR) + goto error; + + ret = poll(&fd, 1, 5);
ret = poll(&fd, 1, MONITOR_POLL_TIMEOUT); Giving up after 5 milliseconds? Are we that afraid of commitment?
+static int qemudWaitForMonitor(struct qemud_vm *vm) {
We don't set an error in here, so how about returning the appropriate errno from the function?
+ char buffer[1024]; /* Plenty of space to get startup greeting */ + int got = 0; + + for (;;) { + int ret; + + ret = read(vm->stderr, buffer+got, sizeof(buffer)-got-1); + if (ret == 0) { + return -1; + } + if (ret < 0) { + struct pollfd fd = { .fd = vm->stderr, .events = POLLIN }; + if (errno != EAGAIN && + errno != EINTR) { + return -1; + } + + ret = poll(&fd, 1, 5000);
Again, a #define for the timeout would be nice
+ if (ret == 0) { + return -1; + } else if (ret < 0) { + if (errno != EINTR) + return -1; + } else if (fd.revents != POLLIN) { + return -1; + } + } else { + char monitor[100]; + char *tmp = buffer; + got += ret; + buffer[got] = '\0'; + while (tmp && *tmp) { + if (sscanf(tmp, "char device redirected to %19s", monitor) == 1) {
Path length of 100, maximum field with of 19? That's all rather voodooish ... hope about strstr() to find it, buffer length of PATH_MAX and then just strncpy()? Also, it'd be nice to split that out into e.g. qemudMonitorPathFromStr(buffer, monitorPath, PATH_MAX)
+ if (qemudOpenMonitor(vm, monitor) < 0) + return -1; + return 0; + } + tmp = index(tmp, '\n');
index() is a bit odd, why not strstr() ?
+static int qemudNextFreeVNCPort(struct qemud_server *server ATTRIBUTE_UNUSED) {
I don't really know the context, but it'd be much nicer if we could just QEMU find a free port itself and then we query the port - e.g. the obvious race condition.
+ int i; + + for (i = 5900 ; i < 6000 ; i++) { + int fd; + struct sockaddr_in addr; + addr.sin_family = AF_INET; + addr.sin_port = htons(i); + addr.sin_addr.s_addr = htonl(INADDR_ANY); + fd = socket(PF_INET, SOCK_STREAM, 0); + if (fd < 0) + return -1; + + if (connect(fd, (struct sockaddr*)&addr, sizeof(addr)) == 0)
Um, why not bind() (with SO_REUSEADDR)?
ret = 0; + + if (qemudWaitForMonitor(vm) < 0) {
Set an error based on the returned errno ...
struct qemud_vm *next = vm->next; @@ -1356,6 +1426,7 @@ static int qemudDispatchPoll(struct qemu if (stderrfd != -1) { if (!failed) { if (fds[fd].revents) { + printf("%d\n", fds[fd].revents);
Kill this Cheers, Mark.