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

Re: [libvirt] [PATCH 3/4] use monitor fd for domain shutdown



Hi,
On Mon, Jan 19, 2009 at 01:38:22PM +0000, Daniel P. Berrange wrote:
> >      /* Got them all, so now open the monitor console */
> > -    ret = qemudOpenMonitor(conn, vm, monitor);
> > +    qemuDriverLock(driver);
> > +    ret = qemudOpenMonitor(conn, driver, vm, monitor, 0);
> > +    qemuDriverUnlock(driver);
> 
> What are the lock/unlock calls here for ? They will cause the whole
> driver to deadlock, because they're violating the rule that a domain
> lock must not be held while acquiring the driver lock. AFAICT in the
> qemudOpenMonitor() method you are just passing the 'driver' object
> straight through to the 'virEventAddHandle' method - since you are
> not using any data fields in it, locking is not required.
I looked at HACKING and couldn't find any explanation of the locking
rules so I added those. They're bogus. Dropped in the new attached
version.

> 
> > @@ -1034,12 +1082,14 @@ static int qemudStartVMDaemon(virConnectPtr conn,
> >          qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s\n"),
> >                   errno, strerror(errno));
> >  
> > -    vm->stdout_fd = -1;
> > -    vm->stderr_fd = -1;
> > +    if ((pos = lseek(vm->logfile, 0, SEEK_END)) < 0)
> > +        qemudLog(QEMUD_WARN, _("Unable to seek to end of logfile %d: %s\n"),
> > +                 errno, strerror(errno));
> >  
> >      for (i = 0 ; i < ntapfds ; i++)
> >          FD_SET(tapfds[i], &keepfd);
> >  
> > +    vm->stderr_fd = vm->stdout_fd = vm->logfile;
> 
> Does anything in the QEMU driver still actually need these stderr_fd / 
> stdout_fd fields after this change ? If not just close the logfile FD
> and leave these initialized to -1 -  we might be able to remove them
> from the virDomainObj struct entirely now because IIRC they're unused
> by LXC / UML drivers too.
O.k.

> > @@ -1202,16 +1207,14 @@ qemudDispatchVMEvent(int watch, int fd, int events, void *opaque) {
> >      if (!vm)
> >          goto cleanup;
> >  
> > -    if (vm->stdout_fd != fd &&
> > -        vm->stderr_fd != fd) {
> > +    if (vm->monitor != fd) {
> >          failed = 1;
> >      } else {
> > -        if (events & VIR_EVENT_HANDLE_READABLE) {
> > -            if (qemudVMData(driver, vm, fd) < 0)
> > -                failed = 1;
> > -        } else {
> > +        if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR))
> >              quit = 1;
> > -        }
> > +        else
> > +            qemudLog(QEMUD_ERROR, _("unhandled fd event %d for %s"),
> > +                                    events, vm->def->name);
> 
> If we get an event in the else scenario there, we should kill the VM
> too, because otherwise we'll just spin endlessly on poll since we're
> not consuming any data (even though its unexpected data!)
Fixed too in the attached version.
 -- Guido
>From ee4b83105a1ca4d75ab7866c61cd5149c8cc6f7f Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Guido=20G=C3=BCnther?= <agx sigxcpu org>
Date: Tue, 20 Jan 2009 08:11:26 +0100
Subject: [PATCH] use monitor fd for domain shutdown

since we dup stdin/stderr on the logfile we need to reparse it for the
monitor path

changes since last time:
 * no need to lock driver object
 * shutdown vm daemon on unhandled vm events
 * don't use stdin_fd, stdout_fd anymore
---
 src/domain_conf.h |    1 +
 src/qemu_driver.c |  156 ++++++++++++++++++++++++++--------------------------
 2 files changed, 79 insertions(+), 78 deletions(-)

diff --git a/src/domain_conf.h b/src/domain_conf.h
index 45b3e10..c236a66 100644
--- a/src/domain_conf.h
+++ b/src/domain_conf.h
@@ -465,6 +465,7 @@ struct _virDomainObj {
     int stderr_fd;
     int stderr_watch;
     int monitor;
+    int monitor_watch;
     char *monitorpath;
     int monitorWatch;
     int logfile;
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index 06f444b..deffb3f 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -181,6 +181,45 @@ qemudLogFD(virConnectPtr conn, const char* logDir, const char* name)
 }
 
 
+static int
+qemudLogReadFD(virConnectPtr conn, const char* logDir, const char* name, off_t pos)
+{
+    char logfile[PATH_MAX];
+    mode_t logmode = O_RDONLY;
+    int ret, fd = -1;
+
+    if ((ret = snprintf(logfile, sizeof(logfile), "%s/%s.log", logDir, name))
+        < 0 || ret >= sizeof(logfile)) {
+        qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                         _("failed to build logfile name %s/%s.log"),
+                         logDir, name);
+        return -1;
+    }
+
+
+    if ((fd = open(logfile, logmode)) < 0) {
+        qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                         _("failed to create logfile %s: %s"),
+                         logfile, strerror(errno));
+        return -1;
+    }
+    if (qemudSetCloseExec(fd) < 0) {
+        qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                         _("Unable to set VM logfile close-on-exec flag: %s"),
+                         strerror(errno));
+        close(fd);
+        return -1;
+    }
+    if (lseek(fd, pos, SEEK_SET) < 0) {
+        qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                         _("Unable to seek to %lld in %s: %s"),
+                         pos, logfile, strerror(errno));
+        close(fd);
+    }
+    return fd;
+}
+
+
 static void
 qemudAutostartConfigs(struct qemud_driver *driver) {
     unsigned int i;
@@ -516,11 +555,7 @@ qemudReadMonitorOutput(virConnectPtr conn,
         int ret;
 
         ret = read(fd, buf+got, buflen-got-1);
-        if (ret == 0) {
-            qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
-                             _("QEMU quit during %s startup\n%s"), what, buf);
-            return -1;
-        }
+
         if (ret < 0) {
             struct pollfd pfd = { .fd = fd, .events = POLLIN };
             if (errno == EINTR)
@@ -584,6 +619,7 @@ qemudCheckMonitorPrompt(virConnectPtr conn ATTRIBUTE_UNUSED,
 }
 
 static int qemudOpenMonitor(virConnectPtr conn,
+                            struct qemud_driver* driver,
                             virDomainObjPtr vm,
                             const char *monitor) {
     int monfd;
@@ -618,6 +654,12 @@ static int qemudOpenMonitor(virConnectPtr conn,
         goto error;
     }
 
+    if ((vm->monitor_watch = virEventAddHandle(vm->monitor, 0,
+                                               qemudDispatchVMEvent,
+                                               driver, NULL)) < 0)
+        goto error;
+
+
     /* Keep monitor open upon success */
     if (ret == 0)
         return ret;
@@ -677,6 +719,7 @@ qemudFindCharDevicePTYs(virConnectPtr conn,
                         const char *output,
                         int fd ATTRIBUTE_UNUSED)
 {
+    struct qemud_driver* driver = conn->privateData;
     char *monitor = NULL;
     size_t offset = 0;
     int ret, i;
@@ -711,7 +754,7 @@ qemudFindCharDevicePTYs(virConnectPtr conn,
     }
 
     /* Got them all, so now open the monitor console */
-    ret = qemudOpenMonitor(conn, vm, monitor);
+    ret = qemudOpenMonitor(conn, driver, vm, monitor, 0);
 
 cleanup:
     VIR_FREE(monitor);
@@ -719,21 +762,23 @@ cleanup:
 }
 
 static int qemudWaitForMonitor(virConnectPtr conn,
-                               virDomainObjPtr vm) {
+                               struct qemud_driver* driver,
+                               virDomainObjPtr vm, off_t pos)
+{
     char buf[1024]; /* Plenty of space to get startup greeting */
-    int ret = qemudReadMonitorOutput(conn,
-                                     vm, vm->stderr_fd,
-                                     buf, sizeof(buf),
-                                     qemudFindCharDevicePTYs,
-                                     "console", 3000);
+    int logfd;
+    int ret;
 
-    buf[sizeof(buf)-1] = '\0';
+    if ((logfd = qemudLogReadFD(conn, driver->logDir, vm->def->name, pos))
+        < 0)
+        return logfd;
 
-    if (safewrite(vm->logfile, buf, strlen(buf)) < 0) {
-        /* Log, but ignore failures to write logfile for VM */
-        qemudLog(QEMUD_WARN, _("Unable to log VM console data: %s\n"),
+    ret = qemudReadMonitorOutput(conn, vm, logfd, buf, sizeof(buf),
+                                 qemudFindCharDevicePTYs,
+                                 "console", 3000);
+    if (close(logfd) < 0)
+        qemudLog(QEMUD_WARN, _("Unable to close logfile: %s\n"),
                  strerror(errno));
-    }
     return ret;
 }
 
@@ -942,6 +987,7 @@ static int qemudStartVMDaemon(virConnectPtr conn,
     fd_set keepfd;
     const char *emulator;
     pid_t child;
+    int pos = -1;
 
     FD_ZERO(&keepfd);
 
@@ -1034,14 +1080,15 @@ static int qemudStartVMDaemon(virConnectPtr conn,
         qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s\n"),
                  errno, strerror(errno));
 
-    vm->stdout_fd = -1;
-    vm->stderr_fd = -1;
+    if ((pos = lseek(vm->logfile, 0, SEEK_END)) < 0)
+        qemudLog(QEMUD_WARN, _("Unable to seek to end of logfile %d: %s\n"),
+                 errno, strerror(errno));
 
     for (i = 0 ; i < ntapfds ; i++)
         FD_SET(tapfds[i], &keepfd);
 
     ret = virExec(conn, argv, progenv, &keepfd, &child,
-                  vm->stdin_fd, &vm->stdout_fd, &vm->stderr_fd,
+                  vm->stdin_fd, &vm->logfile, &vm->logfile,
                   VIR_EXEC_NONBLOCK | VIR_EXEC_DAEMON);
 
     /* wait for qemu process to to show up */
@@ -1078,19 +1125,7 @@ static int qemudStartVMDaemon(virConnectPtr conn,
     }
 
     if (ret == 0) {
-        if (((vm->stdout_watch = virEventAddHandle(vm->stdout_fd,
-                                                   VIR_EVENT_HANDLE_READABLE |
-                                                   VIR_EVENT_HANDLE_ERROR |
-                                                   VIR_EVENT_HANDLE_HANGUP,
-                                                   qemudDispatchVMEvent,
-                                                   driver, NULL)) < 0) ||
-            ((vm->stderr_watch = virEventAddHandle(vm->stderr_fd,
-                                                   VIR_EVENT_HANDLE_READABLE |
-                                                   VIR_EVENT_HANDLE_ERROR |
-                                                   VIR_EVENT_HANDLE_HANGUP,
-                                                   qemudDispatchVMEvent,
-                                                   driver, NULL)) < 0) ||
-            (qemudWaitForMonitor(conn, vm) < 0) ||
+        if ((qemudWaitForMonitor(conn, driver, vm, pos) < 0) ||
             (qemudDetectVcpuPIDs(conn, vm) < 0) ||
             (qemudInitCpus(conn, vm, migrateFrom) < 0)) {
             qemudShutdownVMDaemon(conn, driver, vm);
@@ -1102,32 +1137,6 @@ static int qemudStartVMDaemon(virConnectPtr conn,
     return ret;
 }
 
-static int qemudVMData(struct qemud_driver *driver ATTRIBUTE_UNUSED,
-                       virDomainObjPtr vm, int fd) {
-    char buf[4096];
-    if (vm->pid < 0)
-        return 0;
-
-    for (;;) {
-        int ret = read(fd, buf, sizeof(buf)-1);
-        if (ret < 0) {
-            if (errno == EAGAIN)
-                return 0;
-            return -1;
-        }
-        if (ret == 0) {
-            return 0;
-        }
-        buf[ret] = '\0';
-
-        if (safewrite(vm->logfile, buf, ret) < 0) {
-            /* Log, but ignore failures to write logfile for VM */
-            qemudLog(QEMUD_WARN, _("Unable to log VM console data: %s\n"),
-                     strerror(errno));
-        }
-    }
-}
-
 
 static void qemudShutdownVMDaemon(virConnectPtr conn ATTRIBUTE_UNUSED,
                                   struct qemud_driver *driver, virDomainObjPtr vm) {
@@ -1141,22 +1150,14 @@ static void qemudShutdownVMDaemon(virConnectPtr conn ATTRIBUTE_UNUSED,
         qemudLog(QEMUD_ERROR, _("Failed to send SIGTERM to %s (%d): %s\n"),
                  vm->def->name, vm->pid, strerror(errno));
 
-    qemudVMData(driver, vm, vm->stdout_fd);
-    qemudVMData(driver, vm, vm->stderr_fd);
-
-    virEventRemoveHandle(vm->stdout_watch);
-    virEventRemoveHandle(vm->stderr_watch);
+    virEventRemoveHandle(vm->monitor_watch);
 
     if (close(vm->logfile) < 0)
         qemudLog(QEMUD_WARN, _("Unable to close logfile %d: %s\n"),
                  errno, strerror(errno));
-    close(vm->stdout_fd);
-    close(vm->stderr_fd);
     if (vm->monitor != -1)
         close(vm->monitor);
     vm->logfile = -1;
-    vm->stdout_fd = -1;
-    vm->stderr_fd = -1;
     vm->monitor = -1;
 
     /* shut it off for sure */
@@ -1191,8 +1192,7 @@ qemudDispatchVMEvent(int watch, int fd, int events, void *opaque) {
         virDomainObjPtr tmpvm = driver->domains.objs[i];
         virDomainObjLock(tmpvm);
         if (virDomainIsActive(tmpvm) &&
-            (tmpvm->stdout_watch == watch ||
-             tmpvm->stderr_watch == watch)) {
+            tmpvm->monitor_watch == watch) {
             vm = tmpvm;
             break;
         }
@@ -1202,15 +1202,15 @@ qemudDispatchVMEvent(int watch, int fd, int events, void *opaque) {
     if (!vm)
         goto cleanup;
 
-    if (vm->stdout_fd != fd &&
-        vm->stderr_fd != fd) {
+    if (vm->monitor != fd) {
         failed = 1;
     } else {
-        if (events & VIR_EVENT_HANDLE_READABLE) {
-            if (qemudVMData(driver, vm, fd) < 0)
-                failed = 1;
-        } else {
+        if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR))
             quit = 1;
+        else {
+            qemudLog(QEMUD_ERROR, _("unhandled fd event %d for %s"),
+                                    events, vm->def->name);
+            failed = 1;
         }
     }
 
-- 
1.6.0.6


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