[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