[libvirt] [PATCH 3/4] use monitor fd for domain shutdown
This way we don't have to bother about stdin/stdout/stderr on reconnect. Since we dup stdin/stderr on the logfile we need to reparse it for the monitor path. Cheers, -- Guido --- src/domain_conf.h | 1 + src/qemu_driver.c | 155 +++++++++++++++++++++++++++-------------------------- 2 files changed, 80 insertions(+), 76 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..734a329 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,9 @@ qemudFindCharDevicePTYs(virConnectPtr conn, } /* 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); cleanup: VIR_FREE(monitor); @@ -719,21 +764,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 +989,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, fd_set keepfd; const char *emulator; pid_t child; + int pos = -1; FD_ZERO(&keepfd); @@ -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; ret = virExec(conn, argv, progenv, &keepfd, &child, vm->stdin_fd, &vm->stdout_fd, &vm->stderr_fd, VIR_EXEC_NONBLOCK | VIR_EXEC_DAEMON); @@ -1078,19 +1128,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 +1140,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,17 +1153,11 @@ 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; @@ -1191,8 +1197,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,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 (failed || quit) { -- 1.6.0.6
On Sun, Jan 18, 2009 at 08:28:45PM +0100, Guido G?nther wrote:
This way we don't have to bother about stdin/stdout/stderr on reconnect. Since we dup stdin/stderr on the logfile we need to reparse it for the monitor path.
@@ -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,9 @@ qemudFindCharDevicePTYs(virConnectPtr conn, }
/* 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.
@@ -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.
@@ -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!) Daniel -- |: 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 :|
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
On Tue, Jan 20, 2009 at 08:17:07AM +0100, Guido G?nther wrote:
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.
Yes, I need to write a doc about threading - its too long to put into the HACKING file directly. ACK, this looks good now. Daniel -- |: 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 :|
On Tue, Jan 20, 2009 at 10:21:44AM +0000, Daniel P. Berrange wrote:
On Tue, Jan 20, 2009 at 08:17:07AM +0100, Guido G?nther wrote:
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.
Yes, I need to write a doc about threading - its too long to put into the HACKING file directly.
ACK, this looks good now.
Okay commited for Guido, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Jan 20, 2009 at 08:17:07AM +0100, Guido Günther wrote:
+ 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));
I just had to fix this, this raised a warning about the %lld use to display pos, so I casted to (long long), I assume it's an architecture specific problem (32 vs. 64 bits), Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Guido Günther