[libvirt] [PATCH 3/4] use monitor fd for domain shutdown
Guido Günther
agx at sigxcpu.org
Tue Jan 20 07:17:07 UTC 2009
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-use-monitor-fd-for-domain-shutdown.patch
Type: text/x-diff
Size: 10856 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20090120/e9d962ad/attachment-0001.bin>
More information about the libvir-list
mailing list