[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 5/5] read saved vm status on libvirtd startup



On Mon, Dec 15, 2008 at 11:27:27AM +0000, Daniel P. Berrange wrote:
> NACK, to these 3 lines - since we go to trouble of creating the statefile,
> we should store the FD numbers in the statefile too, instead of relying
> in the linux specific /proc code.
This patch actually monitors things in /proc which is not very portable.
I'll think about something better. Let's leave out this (and the
daemonizing patch for now) until I've cleaned this up.

> > +        if (vm->stdin_fd == -1 || vm->stdout_fd == -1 || vm->stderr_fd == -1)
> > +            goto next_error;
> > +
> > +        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)) {
> 
> It is actually not neccessary to add the _ERROR or _HANGUP flags when
> registering a handle. Those two conditions are implict for every read
> or write watch. Yes, other code in QEMU driver adding them is bogus too.
Ahh, good to know.

> >  /**
> >   * qemudStartup:
> >   *
> > @@ -316,6 +454,7 @@ qemudStartup(void) {
> >                                  qemu_driver->autostartDir,
> >                                  NULL, NULL) < 0)
> >          goto error;
> > +    qemudReconnectVMs(qemu_driver);
> >      qemudAutostartConfigs(qemu_driver);
> >  
> >      qemuDriverUnlock(qemu_driver);
> > @@ -418,11 +557,14 @@ qemudShutdown(void) {
> >  
> >      /* shutdown active VMs */
> >      for (i = 0 ; i < qemu_driver->domains.count ; i++) {
> > +    /* FIXME: don't shutdown VMs on daemon shutdown for now */
> > +#if 0
> >          virDomainObjPtr dom = qemu_driver->domains.objs[i];
> >          virDomainObjLock(dom);
> >          if (virDomainIsActive(dom))
> >              qemudShutdownVMDaemon(NULL, qemu_driver, dom);
> >          virDomainObjUnlock(dom);
> > +#endif
> 
> Yep, just remove this block of code entirely - we dont want to
> shutdown VMs anymore. If someone wants to explicitly save or
> shutdown VMs during machine shutdown, they're probably better
> off explicitly using virsh to do it from an initscript.
Great, I'll simply remove it.
 -- Guido


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]