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