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?= <[email protected]>
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
--
Libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
