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

Re: [Libvir] PATCH: Process QEMU monitor at startup



On Fri, Mar 02, 2007 at 03:34:33PM +0000, Daniel P. Berrange wrote:
> On Fri, Mar 02, 2007 at 03:06:56PM +0000, Mark McLoughlin wrote:
> > > +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?
> 
> I'll add in some error reporting.

We now get errors back like:

$ ./virsh --connect qemu:///session start Fedora
libvir: QEMU error : internal error Domain shutdown before sending monitor PTY path
error: Failed to start domain Fedora


> > > +                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)
> 
> I'll have a go at splitting it out.

This is now done.

> > 	index() is a bit odd, why not strstr() ?
> 
> Well we're only looking for a single character match - index() is for
> single characters, strstr() is for strings.

Changed to use strchr() - although there's several other bits in existing
code which already using 'index()' which also need updating.

> > > +        if (connect(fd, (struct sockaddr*)&addr, sizeof(addr)) == 0)
> > 
> > 	Um, why not bind() (with SO_REUSEADDR)?
> 
> No particular reason - either will work just fine.

Actually one case where bind() is better is if the server is not accepting new
connections - connect() will block indefinitely unless the server actually
promptly accepts them. So I've changed to bind() + REUSEADDR.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 
Index: conf.c
===================================================================
RCS file: /data/cvs/libvirt/qemud/conf.c,v
retrieving revision 1.38
diff -u -p -r1.38 conf.c
--- conf.c	26 Feb 2007 15:34:24 -0000	1.38
+++ conf.c	2 Mar 2007 16:42:49 -0000
@@ -1308,6 +1308,7 @@ qemudAssignVMDef(struct qemud_server *se
     vm->monitor = -1;
     vm->pid = -1;
     vm->id = -1;
+    vm->state = QEMUD_STATE_STOPPED;
     vm->def = def;
     vm->next = server->vms;
 
Index: driver.c
===================================================================
RCS file: /data/cvs/libvirt/qemud/driver.c,v
retrieving revision 1.17
diff -u -p -r1.17 driver.c
--- driver.c	26 Feb 2007 14:21:21 -0000	1.17
+++ driver.c	2 Mar 2007 16:42:49 -0000
@@ -60,6 +60,7 @@ int qemudMonitorCommand(struct qemud_ser
                         char **reply) {
     int size = 0;
     char *buf = NULL;
+
     if (write(vm->monitor, cmd, strlen(cmd)) < 0) {
         return -1;
     }
@@ -74,13 +75,20 @@ int qemudMonitorCommand(struct qemud_ser
         for (;;) {
             char data[1024];
             int got = read(vm->monitor, data, sizeof(data));
+
+            if (got == 0) {
+                if (buf)
+                    free(buf);
+                return -1;
+            }
             if (got < 0) {
                 if (errno == EINTR)
                     continue;
                 if (errno == EAGAIN)
                     break;
 
-                free(buf);
+                if (buf)
+                    free(buf);
                 return -1;
             }
             if (!(buf = realloc(buf, size+got+1)))
@@ -92,7 +100,7 @@ int qemudMonitorCommand(struct qemud_ser
         if (buf)
             qemudDebug("Mon [%s]", buf);
         /* Look for QEMU prompt to indicate completion */
-        if (buf && ((tmp = strstr(buf, "\n(qemu)")) != NULL)) {
+        if (buf && ((tmp = strstr(buf, "\n(qemu) ")) != NULL)) {
             tmp[0] = '\0';
             break;
         }
@@ -314,11 +322,14 @@ int qemudDomainSuspend(struct qemud_serv
         qemudReportError(server, VIR_ERR_OPERATION_FAILED, "domain is not running");
         return -1;
     }
+    if (vm->state == QEMUD_STATE_PAUSED)
+        return 0;
 
     if (qemudMonitorCommand(server, vm, "stop\n", &info) < 0) {
         qemudReportError(server, VIR_ERR_OPERATION_FAILED, "suspend operation failed");
         return -1;
     }
+    vm->state = QEMUD_STATE_PAUSED;
     qemudDebug("Reply %s", info);
     free(info);
     return 0;
@@ -336,13 +347,16 @@ int qemudDomainResume(struct qemud_serve
         qemudReportError(server, VIR_ERR_OPERATION_FAILED, "domain is not running");
         return -1;
     }
+    if (vm->state == QEMUD_STATE_RUNNING)
+        return 0;
     if (qemudMonitorCommand(server, vm, "cont\n", &info) < 0) {
         qemudReportError(server, VIR_ERR_OPERATION_FAILED, "resume operation failed");
         return -1;
     }
+    vm->state = QEMUD_STATE_RUNNING;
     qemudDebug("Reply %s", info);
     free(info);
-    return -1;
+    return 0;
 }
 
 
@@ -371,12 +385,7 @@ int qemudDomainGetInfo(struct qemud_serv
         return -1;
     }
 
-    if (!qemudIsActiveVM(vm)) {
-        *runstate = QEMUD_STATE_STOPPED;
-    } else {
-        /* XXX in future need to add PAUSED */
-        *runstate = QEMUD_STATE_RUNNING;
-    }
+    *runstate = vm->state;
 
     if (!qemudIsActiveVM(vm)) {
         *cputime = 0;
@@ -432,7 +441,7 @@ int qemudDomainDumpXML(struct qemud_serv
     strncpy(xml, vmxml, xmllen);
     xml[xmllen-1] = '\0';
 
-    free(xml);
+    free(vmxml);
 
     return 0;
 }
Index: internal.h
===================================================================
RCS file: /data/cvs/libvirt/qemud/internal.h,v
retrieving revision 1.17
diff -u -p -r1.17 internal.h
--- internal.h	23 Feb 2007 17:15:18 -0000	1.17
+++ internal.h	2 Mar 2007 16:42:49 -0000
@@ -216,6 +216,7 @@ struct qemud_vm {
     int monitor;
     int pid;
     int id;
+    int state;
 
     int *tapfds;
     int ntapfds;
Index: qemud.c
===================================================================
RCS file: /data/cvs/libvirt/qemud/qemud.c,v
retrieving revision 1.29
diff -u -p -r1.29 qemud.c
--- qemud.c	23 Feb 2007 17:15:18 -0000	1.29
+++ qemud.c	2 Mar 2007 16:42:50 -0000
@@ -43,6 +43,7 @@
 #include <string.h>
 #include <errno.h>
 #include <getopt.h>
+#include <ctype.h>
 
 #include <libvirt/virterror.h>
 
@@ -614,6 +615,197 @@ qemudExec(struct qemud_server *server, c
     return -1;
 }
 
+#define MONITOR_TIMEOUT 3000
+
+static int qemudOpenMonitor(struct qemud_server *server, struct qemud_vm *vm, const char *monitor) {
+    int monfd;
+    char buffer[1024];
+    int got = 0;
+
+    if (!(monfd = open(monitor, O_RDWR))) {
+        qemudReportError(server, VIR_ERR_INTERNAL_ERROR,
+                         "Unable to open monitor path %s", monitor);
+        return -1;
+    }
+    if (qemudSetCloseExec(monfd) < 0) {
+        qemudReportError(server, VIR_ERR_INTERNAL_ERROR,
+                         "Unable to set monitor close-on-exec flag");
+        goto error;
+    }
+    if (qemudSetNonBlock(monfd) < 0) {
+        qemudReportError(server, VIR_ERR_INTERNAL_ERROR,
+                         "Unable to put monitor into non-blocking mode");
+        goto error;
+    }
+
+    /* Consume & discard the initial greeting */
+    while (got < (sizeof(buffer)-1)) {
+        int ret;
+
+        ret = read(monfd, buffer+got, sizeof(buffer)-got-1);
+        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, MONITOR_TIMEOUT);
+            if (ret == 0) {
+                qemudReportError(server, VIR_ERR_INTERNAL_ERROR,
+                                 "Timed out while waiting for monitor prompt");
+                goto error;
+            } else if (ret == -1) {
+                if (errno != EINTR) {
+                    qemudReportError(server, VIR_ERR_INTERNAL_ERROR,
+                                     "Failure while waiting for monitor prompt");
+                    goto error;
+                }
+            } else if (fd.revents != POLLIN) {
+                qemudReportError(server, VIR_ERR_INTERNAL_ERROR,
+                                 "Failure while waiting for monitor prompt");
+                goto error;
+            }
+        } else {
+            got += ret;
+            buffer[got] = '\0';
+            if (strstr(buffer, "(qemu) ") != NULL) {
+                vm->monitor = monfd;
+                return 0;
+            }
+        }
+    }
+
+    qemudReportError(server, VIR_ERR_INTERNAL_ERROR,
+                     "No monitor prompt found");
+
+ error:
+    close(monfd);
+    return -1;
+}
+
+static int qemudExtractMonitorPath(const char *haystack, char *path, int pathmax) {
+    static const char needle[] = "char device redirected to";
+    char *tmp;
+
+    if (!(tmp = strstr(haystack, needle)))
+        return -1;
+
+    strncpy(path, tmp+sizeof(needle), pathmax-1);
+    path[pathmax-1] = '\0';
+
+    while (*path) {
+        /*
+         * The monitor path ends at first whitespace char
+         * so lets search for it & NULL terminate it there
+         */
+        if (isspace(*path)) {
+            *path = '\0';
+            return 0;
+        }
+        path++;
+    }
+
+    /*
+     * We found a path, but didn't find any whitespace,
+     * so it must be still incomplete - we should at
+     * least see a \n
+     */
+    return -1;
+}
+
+static int qemudWaitForMonitor(struct qemud_server *server, struct qemud_vm *vm) {
+    char buffer[1024]; /* Plenty of space to get startup greeting */
+    int got = 0;
+
+    while (got < (sizeof(buffer)-1)) {
+        int ret;
+
+        ret = read(vm->stderr, buffer+got, sizeof(buffer)-got-1);
+        if (ret == 0) {
+            break; /* End of file */
+        }
+        if (ret < 0) {
+            struct pollfd fd = { .fd = vm->stderr, .events = POLLIN };
+            if (errno != EAGAIN &&
+                errno != EINTR) {
+                qemudReportError(server, VIR_ERR_INTERNAL_ERROR,
+                                 "Failure while looking for monitor PTY path");
+                return -1;
+            }
+
+            ret = poll(&fd, 1, MONITOR_TIMEOUT);
+            if (ret == 0) {
+                qemudReportError(server, VIR_ERR_INTERNAL_ERROR,
+                                 "Timed out while waiting for monitor PTY path");
+                return -1;
+            } else if (ret < 0) {
+                if (errno != EINTR) {
+                    qemudReportError(server, VIR_ERR_INTERNAL_ERROR,
+                                     "Failure while looking for monitor PTY path");
+                    return -1;
+                }
+            } else if (fd.revents & POLLHUP) {
+                break; /* End of file */
+            } else if (fd.revents != POLLIN) {
+                qemudReportError(server, VIR_ERR_INTERNAL_ERROR,
+                                 "Failure while looking for monitor PTY path");
+                return -1;
+            }
+        } else {
+            char monitor[PATH_MAX];
+            got += ret;
+            buffer[got] = '\0';
+
+            if (qemudExtractMonitorPath(buffer, monitor, sizeof(monitor)) == 0) {
+                if (qemudOpenMonitor(server, vm, monitor) < 0)
+                    return -1;
+                return 0;
+            }
+        }
+    }
+
+    qemudReportError(server, VIR_ERR_INTERNAL_ERROR,
+                     "Domain shutdown before sending monitor PTY path");
+    return -1;
+}
+
+static int qemudNextFreeVNCPort(struct qemud_server *server ATTRIBUTE_UNUSED) {
+    int i;
+
+    for (i = 5900 ; i < 6000 ; i++) {
+        int fd;
+        int reuse = 1;
+        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 (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (void*)&reuse, sizeof(reuse)) < 0) {
+            close(fd);
+            break;
+        }
+
+        if (bind(fd, (struct sockaddr*)&addr, sizeof(addr)) == 0) {
+            /* Not in use, lets grab it */
+            close(fd);
+            return i;
+        }
+        close(fd);
+
+        if (errno == EADDRINUSE) {
+            /* In use, try next */
+            continue;
+        }
+        /* Some other bad failure, get out.. */
+        break;
+    }
+    return -1;
+}
 
 int qemudStartVMDaemon(struct qemud_server *server,
                        struct qemud_vm *vm) {
@@ -626,9 +818,15 @@ int qemudStartVMDaemon(struct qemud_serv
         return -1;
     }
 
-    if (vm->def->vncPort < 0)
-        vm->def->vncActivePort = 5900 + server->nextvmid;
-    else
+    if (vm->def->vncPort < 0) {
+        int port = qemudNextFreeVNCPort(server);
+        if (port < 0) {
+            qemudReportError(server, VIR_ERR_INTERNAL_ERROR,
+                             "Unable to find an unused VNC port");
+            return -1;
+        }
+        vm->def->vncActivePort = port;
+    } else
         vm->def->vncActivePort = vm->def->vncPort;
 
     if (qemudBuildCommandLine(server, vm, &argv) < 0)
@@ -636,12 +834,18 @@ int qemudStartVMDaemon(struct qemud_serv
 
     if (qemudExec(server, argv, &vm->pid, &vm->stdout, &vm->stderr) == 0) {
         vm->id = server->nextvmid++;
+        vm->state = QEMUD_STATE_RUNNING;
 
         server->ninactivevms--;
         server->nactivevms++;
         server->nvmfds += 2;
 
         ret = 0;
+
+        if (qemudWaitForMonitor(server, vm) < 0) {
+            qemudShutdownVMDaemon(server, vm);
+            ret = -1;
+        }
     }
 
     if (vm->tapfds) {
@@ -804,50 +1008,6 @@ static int qemudVMData(struct qemud_serv
         }
         buf[ret] = '\0';
 
-        /*
-         * XXX this is bad - we should wait for tty and open the
-         * monitor when actually starting the guest, so we can
-         * reliably trap startup failures
-         */
-        if (vm->monitor == -1) {
-            char monitor[20];
-            /* Fairly lame assuming we receive the data all in one chunk.
-               This isn't guarenteed, but in practice it seems good enough.
-               This will probably bite me in the future.... */
-            if (sscanf(buf, "char device redirected to %19s", monitor) == 1) {
-                int monfd;
-
-                if (!(monfd = open(monitor, O_RDWR))) {
-                    perror("cannot open monitor");
-                    return -1;
-                }
-                if (qemudSetCloseExec(monfd) < 0) {
-                    close(monfd);
-                    return -1;
-                }
-                if (qemudSetNonBlock(monfd) < 0) {
-                    close(monfd);
-                    return -1;
-                }
-
-                /* Consume & discard the initial greeting */
-                /* XXX this is broken, we need to block until
-                   we see the initial prompt to ensure startup
-                   has completed */
-                for(;;) {
-                    char line[1024];
-                    if (read(monfd, line, sizeof(line)) < 0) {
-                        if (errno == EAGAIN) {
-                            break;
-                        }
-                        close(monfd);
-                        return -1;
-                    }
-                    qemudDebug("[%s]", line);
-                }
-                vm->monitor = monfd;
-            }
-        }
         qemudDebug("[%s]", buf);
     }
 }
@@ -896,6 +1056,7 @@ int qemudShutdownVMDaemon(struct qemud_s
 
     vm->pid = -1;
     vm->id = -1;
+    vm->state = QEMUD_STATE_STOPPED;
 
     if (vm->newDef) {
         qemudFreeVMDef(vm->def);
@@ -1305,30 +1466,6 @@ static int qemudDispatchPoll(struct qemu
     if (server->shutdown)
         return 0;
 
-    while (sock) {
-        struct qemud_socket *next = sock->next;
-        /* FIXME: the daemon shouldn't exit on error here */
-        if (fds[fd].revents)
-            if (qemudDispatchServer(server, sock) < 0)
-                return -1;
-        fd++;
-        sock = next;
-    }
-
-    while (client) {
-        struct qemud_client *next = client->next;
-        if (fds[fd].revents) {
-            qemudDebug("Poll data normal");
-            if (fds[fd].revents == POLLOUT)
-                qemudDispatchClientWrite(server, client);
-            else if (fds[fd].revents == POLLIN)
-                qemudDispatchClientRead(server, client);
-            else
-                qemudDispatchClientFailure(server, client);
-        }
-        fd++;
-        client = next;
-    }
     vm = server->vms;
     while (vm) {
         struct qemud_vm *next = vm->next;
@@ -1371,6 +1508,29 @@ static int qemudDispatchPoll(struct qemu
         if (failed)
             ret = -1; /* FIXME: the daemon shouldn't exit on failure here */
     }
+    while (client) {
+        struct qemud_client *next = client->next;
+        if (fds[fd].revents) {
+            qemudDebug("Poll data normal");
+            if (fds[fd].revents == POLLOUT)
+                qemudDispatchClientWrite(server, client);
+            else if (fds[fd].revents == POLLIN)
+                qemudDispatchClientRead(server, client);
+            else
+                qemudDispatchClientFailure(server, client);
+        }
+        fd++;
+        client = next;
+    }
+    while (sock) {
+        struct qemud_socket *next = sock->next;
+        /* FIXME: the daemon shouldn't exit on error here */
+        if (fds[fd].revents)
+            if (qemudDispatchServer(server, sock) < 0)
+                return -1;
+        fd++;
+        sock = next;
+    }
 
     /* Cleanup any VMs which shutdown & dont have an associated
        config file */
@@ -1408,22 +1568,6 @@ static void qemudPreparePoll(struct qemu
     fds[fd].events = POLLIN;
     fd++;
 
-    for (sock = server->sockets ; sock ; sock = sock->next) {
-        fds[fd].fd = sock->fd;
-        fds[fd].events = POLLIN;
-        fd++;
-    }
-
-    for (client = server->clients ; client ; client = client->next) {
-        fds[fd].fd = client->fd;
-        /* Refuse to read more from client if tx is pending to
-           rate limit */
-        if (client->tx)
-            fds[fd].events = POLLOUT | POLLERR | POLLHUP;
-        else
-            fds[fd].events = POLLIN | POLLERR | POLLHUP;
-        fd++;
-    }
     for (vm = server->vms ; vm ; vm = vm->next) {
         if (!qemudIsActiveVM(vm))
             continue;
@@ -1438,6 +1582,21 @@ static void qemudPreparePoll(struct qemu
             fd++;
         }
     }
+    for (client = server->clients ; client ; client = client->next) {
+        fds[fd].fd = client->fd;
+        /* Refuse to read more from client if tx is pending to
+           rate limit */
+        if (client->tx)
+            fds[fd].events = POLLOUT | POLLERR | POLLHUP;
+        else
+            fds[fd].events = POLLIN | POLLERR | POLLHUP;
+        fd++;
+    }
+    for (sock = server->sockets ; sock ; sock = sock->next) {
+        fds[fd].fd = sock->fd;
+        fds[fd].events = POLLIN;
+        fd++;
+    }
 }
 
 

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