[libvirt] PATCH: Misc user mode linux startup bugs

[Daniel P. Berrange]

There's a number of annoying bugs in the user mode linux driver startup
code. This patch fixes lots of them. We failed to check umlMonitorCommand
for error, leading to a NULL de-reference due to another bug in that
function. This patch also checks if 'res' is NULL before de-referenceing
it, just in case.  In the code for handling inotify delete events, we
duplicated alot of the umlShutdownVMDaemon code, but forgot to close
the monitor. This patches makes it just call umlShutdownVMDaemon directly.
In one place where we spin in a usleep loop, we forgot to increment our
retries counter, making it spin forever. Finally, switch to virKillProcess
and make the latter also avoids pid==1, since I can't  imagine a time 
where we'd ever want to kill 'init' :-)

Daniel

diff --git a/src/uml_driver.c b/src/uml_driver.c
--- a/src/uml_driver.c
+++ b/src/uml_driver.c
@@ -166,9 +166,10 @@ umlIdentifyOneChrPTY(virConnectPtr conn,
         return -1;
     }
 requery:
-    umlMonitorCommand(NULL, driver, dom, cmd, &res);
+    if (umlMonitorCommand(NULL, driver, dom, cmd, &res) < 0)
+        return -1;
 
-    if (STRPREFIX(res, "pts:")) {
+    if (res && STRPREFIX(res, "pts:")) {
         VIR_FREE(def->data.file.path);
         if ((def->data.file.path = strdup(res + 4)) == NULL) {
             virReportOOMError(conn);
@@ -176,7 +177,7 @@ requery:
             VIR_FREE(cmd);
             return -1;
         }
-    } else if (STRPREFIX(res, "pts")) {
+    } else if (!res || STRPREFIX(res, "pts")) {
         /* It can take a while to startup, so retry for
            upto 5 seconds */
         /* XXX should do this in a better non-blocking
@@ -263,19 +264,15 @@ reread:
         }
 
         if (e->mask & IN_DELETE) {
+            VIR_DEBUG("Got inotify domain shutdown '%s'", name);
             if (!virDomainIsActive(dom)) {
                 virDomainObjUnlock(dom);
                 continue;
             }
 
-            dom->def->id = -1;
-            dom->pid = -1;
-            if (dom->newDef) {
-                virDomainDefFree(dom->def);
-                dom->def = dom->newDef;
-            }
-            dom->state = VIR_DOMAIN_SHUTOFF;
+            umlShutdownVMDaemon(NULL, driver, dom);
         } else if (e->mask & (IN_CREATE | IN_MODIFY)) {
+            VIR_DEBUG("Got inotify domain startup '%s'", name);
             if (virDomainIsActive(dom)) {
                 virDomainObjUnlock(dom);
                 continue;
@@ -289,11 +286,13 @@ reread:
             dom->def->id = driver->nextvmid++;
             dom->state = VIR_DOMAIN_RUNNING;
 
-            if (umlOpenMonitor(NULL, driver, dom) < 0)
+            if (umlOpenMonitor(NULL, driver, dom) < 0) {
+                VIR_WARN0("Could not open monitor for new domain");
                 umlShutdownVMDaemon(NULL, driver, dom);
-
-            if (umlIdentifyChrPTY(NULL, driver, dom) < 0)
+            } else if (umlIdentifyChrPTY(NULL, driver, dom) < 0) {
+                VIR_WARN0("Could not identify charater devices for new domain");
                 umlShutdownVMDaemon(NULL, driver, dom);
+            }
         }
         virDomainObjUnlock(dom);
     }
@@ -383,6 +382,7 @@ umlStartup(void) {
         goto error;
     }
 
+    VIR_INFO("Adding inotify watch on %s", uml_driver->monitorDir);
     if (inotify_add_watch(uml_driver->inotifyFD,
                           uml_driver->monitorDir,
                           IN_CREATE | IN_MODIFY | IN_DELETE) < 0) {
@@ -595,10 +595,11 @@ static int umlOpenMonitor(virConnectPtr 
     if (umlMonitorAddress(conn, driver, vm, &addr) < 0)
         return -1;
 
+    VIR_DEBUG("Dest address for monitor is '%s'", addr.sun_path);
 restat:
     if (stat(addr.sun_path, &sb) < 0) {
         if (errno == ENOENT &&
-            retries < 50) {
+            retries++ < 50) {
             usleep(1000 * 100);
             goto restat;
         }
@@ -612,7 +613,8 @@ restat:
     }
 
     memset(addr.sun_path, 0, sizeof addr.sun_path);
-    sprintf(addr.sun_path + 1, "%u", getpid());
+    sprintf(addr.sun_path + 1, "libvirt-uml-%u", vm->pid);
+    VIR_DEBUG("Reply address for monitor is '%s'", addr.sun_path+1);
     if (bind(vm->monitor, (struct sockaddr *)&addr, sizeof addr) < 0) {
         virReportSystemError(conn, errno,
                              "%s", _("cannot bind socket"));
@@ -656,6 +658,8 @@ static int umlMonitorCommand(virConnectP
     int retlen = 0, ret = 0;
     struct sockaddr_un addr;
     unsigned int addrlen;
+
+    VIR_DEBUG("Run command '%s'", cmd);
 
     *reply = NULL;
 
@@ -706,6 +710,8 @@ static int umlMonitorCommand(virConnectP
 
     } while (res.extra);
 
+    VIR_DEBUG("Command reply is '%s'", NULLSTR(retdata));
+
     *reply = retdata;
 
     return ret;
@@ -852,12 +881,10 @@ static void umlShutdownVMDaemon(virConne
                                 virDomainObjPtr vm)
 {
     int ret;
-    if (!virDomainIsActive(vm) ||
-        vm->pid <= 1)
+    if (!virDomainIsActive(vm))
         return;
 
-
-    kill(vm->pid, SIGTERM);
+    virKillProcess(vm->pid, SIGTERM);
 
     if (vm->monitor != -1)
         close(vm->monitor);
diff --git a/src/util.c b/src/util.c
--- a/src/util.c
+++ b/src/util.c
@@ -1680,7 +1680,7 @@ char *virGetHostname(void)
 /* send signal to a single process */
 int virKillProcess(pid_t pid, int sig)
 {
-    if (pid < 1) {
+    if (pid <= 1) {
         errno = ESRCH;
         return -1;
     }

-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|