[libvirt] [PATCH 3/4] use monitor fd for domain shutdown

[Daniel P. Berrange]

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 :|