Make use of private data structure for monitor state

Introduce a new qemuDomainObjPrivate object which is used to store
the private QEMU specific data associated with each virDomainObjPtr
instance. This contains a single member, an instance of the new
qemuMonitorPtr object which encapsulates the QEMU monitor state.
The internals of the latter are private to the qemu_monitor* files,
not to be shown to qemu_driver.c

* src/qemu/qemu_conf.h: Definition of qemuDomainObjPrivate.
* src/qemu/qemu_driver.c: Register a functions for creating
  and freeing qemuDomainObjPrivate instances with the domain
  capabilities. Remove the qemudDispatchVMEvent() watch since
  I/O watches are now handled by the monitor code itself. Pass
  a new qemuHandleMonitorEOF() callback into qemuMonitorOpen
  to allow notification when the monitor quits.
* src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h: Introduce
  the 'qemuMonitor' object. Temporarily add new APIs
  qemuMonitorWrite, qemuMonitorRead, qemuMonitorWaitForInput
  to allow text based monitor impl to perform I/O.
* src/qemu/qemu_monitor_text.c: Call APIs for reading/writing
  to monitor instead of accessing the file handle directly.

src/qemu/qemu_conf.h

@@ -37,6 +37,7 @@
 #include "security/security_driver.h"
 #include "cgroup.h"
 #include "pci.h"
+#include "qemu_monitor.h"
 
 #define qemudDebug(fmt, ...) do {} while(0)
 
@@ -133,6 +134,16 @@ struct qemud_driver {
     pciDeviceList *activePciHostdevs;
 };
 
+/* XXX temporarily exposed.
+ * This will be moved back into qemu_driver.c, once the
+ * qemu_monitor* code is refactored a little more
+ */
+typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate;
+typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr;
+struct _qemuDomainObjPrivate {
+    qemuMonitorPtr mon;
+};
+
 
 /* Port numbers used for KVM migration. */
 #define QEMUD_MIGRATION_FIRST_PORT 49152

src/qemu/qemu_driver.c

@@ -93,11 +93,6 @@ static void qemuDomainEventFlush(int timer, void *opaque);
 static void qemuDomainEventQueue(struct qemud_driver *driver,
                                  virDomainEventPtr event);
 
-static void qemudDispatchVMEvent(int watch,
-                                 int fd,
-                                 int events,
-                                 void *opaque);
-
 static int qemudStartVMDaemon(virConnectPtr conn,
                               struct qemud_driver *driver,
                               virDomainObjPtr vm,
@@ -118,6 +113,30 @@ static int qemuUpdateActivePciHostdevs(struct qemud_driver *driver,
 
 static struct qemud_driver *qemu_driver = NULL;
 
+
+static void *qemuDomainObjPrivateAlloc(void)
+{
+    qemuDomainObjPrivatePtr priv;
+
+    if (VIR_ALLOC(priv) < 0)
+        return NULL;
+
+    return priv;
+}
+
+static void qemuDomainObjPrivateFree(void *data)
+{
+    qemuDomainObjPrivatePtr priv = data;
+
+    /* This should never be non-NULL if we get here, but just in case... */
+    if (priv->mon) {
+        VIR_ERROR0("Unexpected QEMU monitor still active during domain deletion");
+        qemuMonitorClose(priv->mon);
+    }
+    VIR_FREE(priv);
+}
+
+
 static int qemuCgroupControllerActive(struct qemud_driver *driver,
                                       int controller)
 {
@@ -292,22 +311,54 @@ qemudRemoveDomainStatus(virConnectPtr conn,
     return 0;
 }
 
+
+/*
+ * This is a callback registered with a qemuMonitorPtr  instance,
+ * and to be invoked when the monitor console hits an end of file
+ * condition, or error, thus indicating VM shutdown should be
+ * performed
+ */
+static void
+qemuHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
+                     virDomainObjPtr vm,
+                     int hasError) {
+    struct qemud_driver *driver = qemu_driver;
+    virDomainEventPtr event = NULL;
+
+    qemuDriverLock(driver);
+    virDomainObjLock(vm);
+    qemuDriverUnlock(driver);
+
+    event = virDomainEventNewFromObj(vm,
+                                     VIR_DOMAIN_EVENT_STOPPED,
+                                     hasError ?
+                                     VIR_DOMAIN_EVENT_STOPPED_FAILED :
+                                     VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN);
+
+    qemudShutdownVMDaemon(NULL, driver, vm);
+    if (!vm->persistent)
+        virDomainRemoveInactive(&driver->domains, vm);
+    else
+        virDomainObjUnlock(vm);
+
+    if (event) {
+        qemuDriverLock(driver);
+        qemuDomainEventQueue(driver, event);
+        qemuDriverUnlock(driver);
+    }
+}
+
+
 static int
 qemuConnectMonitor(virDomainObjPtr vm, int reconnect)
 {
-    int rc;
+    qemuDomainObjPrivatePtr priv = vm->privateData;
-    if ((rc = qemuMonitorOpen(vm, reconnect)) != 0) {
+
-        VIR_ERROR(_("Failed to connect monitor for %s: %d\n"),
+    if ((priv->mon = qemuMonitorOpen(vm, reconnect, qemuHandleMonitorEOF)) == NULL) {
-                  vm->def->name, rc);
+        VIR_ERROR(_("Failed to connect monitor for %s\n"), vm->def->name);
         return -1;
     }
 
-    if ((vm->monitorWatch = virEventAddHandle(vm->monitor,
-                                              VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR,
-                                              qemudDispatchVMEvent,
-                                              vm, NULL)) < 0)
-        return -1;
-
     return 0;
 }
 
@@ -351,7 +402,7 @@ error:
 }
 
 /**
- * qemudReconnectVMs
+ * qemudReconnectDomains
  *
  * Try to re-open the resources for live VMs that we care
  * about.
@@ -549,6 +600,9 @@ qemudStartup(int privileged) {
     if ((qemu_driver->caps = qemudCapsInit(NULL)) == NULL)
         goto out_of_memory;
 
+    qemu_driver->caps->privateDataAllocFunc = qemuDomainObjPrivateAlloc;
+    qemu_driver->caps->privateDataFreeFunc = qemuDomainObjPrivateFree;
+
     if ((qemu_driver->activePciHostdevs = pciDeviceListNew(NULL)) == NULL)
         goto error;
 
@@ -1952,6 +2006,7 @@ static void qemudShutdownVMDaemon(virConnectPtr conn,
                                   virDomainObjPtr vm) {
     int ret;
     int retries = 0;
+    qemuDomainObjPrivatePtr priv = vm->privateData;
 
     if (!virDomainObjIsActive(vm))
         return;
@@ -1980,15 +2035,11 @@ static void qemudShutdownVMDaemon(virConnectPtr conn,
                              _("Failed to send SIGTERM to %s (%d)"),
                              vm->def->name, vm->pid);
 
-    if (vm->monitorWatch != -1) {
+    if (priv->mon) {
-        virEventRemoveHandle(vm->monitorWatch);
+        qemuMonitorClose(priv->mon);
-        vm->monitorWatch = -1;
+        priv->mon = NULL;
     }
 
-    if (vm->monitor != -1)
-        close(vm->monitor);
-    vm->monitor = -1;
-
     if (vm->monitor_chr) {
         if (vm->monitor_chr->type == VIR_DOMAIN_CHR_TYPE_UNIX)
             unlink(vm->monitor_chr->data.nix.path);
@@ -2043,59 +2094,6 @@ retry:
 }
 
 
-static void
-qemudDispatchVMEvent(int watch, int fd, int events, void *opaque) {
-    struct qemud_driver *driver = qemu_driver;
-    virDomainObjPtr vm = opaque;
-    virDomainEventPtr event = NULL;
-    int quit = 0, failed = 0;
-
-    /* XXX Normally we have to lock the driver first, to protect
-     * against someone adding/removing the domain. We know,
-     * however, then if we're getting data in this callback
-     * the VM must be running. Nowhere is allowed to remove
-     * a domain while it is running, so it is safe to not
-     * lock the driver here... */
-    qemuDriverLock(driver);
-    virDomainObjLock(vm);
-    qemuDriverUnlock(driver);
-
-    if (vm->monitor != fd || vm->monitorWatch != watch) {
-        failed = 1;
-    } else {
-        if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR))
-            quit = 1;
-        else {
-            VIR_ERROR(_("unhandled fd event %d for %s"),
-                      events, vm->def->name);
-            failed = 1;
-        }
-    }
-
-    if (failed || quit) {
-        event = virDomainEventNewFromObj(vm,
-                                         VIR_DOMAIN_EVENT_STOPPED,
-                                         quit ?
-                                         VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN :
-                                         VIR_DOMAIN_EVENT_STOPPED_FAILED);
-        qemudShutdownVMDaemon(NULL, driver, vm);
-        if (!vm->persistent) {
-            virDomainRemoveInactive(&driver->domains,
-                                    vm);
-            vm = NULL;
-        }
-    }
-
-    virDomainObjUnlock(vm);
-    if (event) {
-        qemuDriverLock(driver);
-        qemuDomainEventQueue(driver, event);
-        qemuDriverUnlock(driver);
-    }
-}
-
-
-
 static virDrvOpenStatus qemudOpen(virConnectPtr conn,
                                   virConnectAuthPtr auth ATTRIBUTE_UNUSED,
                                   int flags ATTRIBUTE_UNUSED) {
@@ -2229,6 +2227,9 @@ static char *qemudGetCapabilities(virConnectPtr conn) {
         goto cleanup;
     }
 
+    caps->privateDataAllocFunc = qemuDomainObjPrivateAlloc;
+    caps->privateDataFreeFunc = qemuDomainObjPrivateFree;
+
     if (qemu_driver->securityDriver &&
         qemudSecurityCapsInit(qemu_driver->securityDriver, caps) < 0) {
         virCapabilitiesFree(caps);

src/qemu/qemu_monitor.c

@@ -32,13 +32,24 @@
 #include "qemu_conf.h"
 #include "event.h"
 #include "virterror_internal.h"
+#include "memory.h"
+#include "logging.h"
 
 #define VIR_FROM_THIS VIR_FROM_QEMU
 
+struct _qemuMonitor {
+    int fd;
+    int watch;
+    int hasSendFD;
+
+    virDomainObjPtr vm;
+
+    qemuMonitorEOFNotify eofCB;
+};
+
 /* Return -1 for error, 1 to continue reading and 0 for success */
 typedef int qemuMonitorHandleOutput(virDomainObjPtr vm,
-                                    const char *output,
+                                    const char *output);
-                                    int fd);
 
 /*
  * Returns -1 for error, 0 on end-of-file, 1 for success
@@ -101,7 +112,7 @@ qemuMonitorReadOutput(virDomainObjPtr vm,
         } else {
             got += ret;
             buf[got] = '\0';
-            ret = func(vm, buf, fd);
+            ret = func(vm, buf);
             if (ret == -1)
                 return -1;
             if (ret == 1)
@@ -117,15 +128,12 @@ qemuMonitorReadOutput(virDomainObjPtr vm,
 }
 
 static int
-qemuMonitorCheckPrompt(virDomainObjPtr vm,
+qemuMonitorCheckPrompt(virDomainObjPtr vm ATTRIBUTE_UNUSED,
-                       const char *output,
+                       const char *output)
-                       int fd)
 {
     if (strstr(output, "(qemu) ") == NULL)
         return 1; /* keep reading */
 
-    vm->monitor = fd;
-
     return 0;
 }
 
@@ -157,14 +165,10 @@ qemuMonitorOpenCommon(virDomainObjPtr vm,
         else
             ret = 0;
     } else {
-        vm->monitor = monfd;
         ret = 0;
     }
 
-    if (ret != 0)
+    return ret;
-        return ret;
-
-    return 0;
 }
 
 static int
@@ -218,7 +222,7 @@ qemuMonitorOpenUnix(virDomainObjPtr vm,
     if (qemuMonitorOpenCommon(vm, monfd, reconnect) < 0)
         goto error;
 
-    return 0;
+    return monfd;
 
 error:
     close(monfd);
@@ -241,28 +245,168 @@ qemuMonitorOpenPty(virDomainObjPtr vm,
     if (qemuMonitorOpenCommon(vm, monfd, reconnect) < 0)
         goto error;
 
-    return 0;
+    return monfd;
 
 error:
     close(monfd);
     return -1;
 }
 
-int
+
+static void
+qemuMonitorIO(int watch, int fd, int events, void *opaque) {
+    qemuMonitorPtr mon = opaque;
+    int quit = 0, failed = 0;
+
+    if (mon->fd != fd || mon->watch != watch) {
+        VIR_ERROR0(_("event from unexpected fd/watch"));
+        failed = 1;
+    } else {
+        if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR))
+            quit = 1;
+        else {
+            VIR_ERROR(_("unhandled fd event %d for monitor fd %d"),
+                      events, mon->fd);
+            failed = 1;
+        }
+    }
+
+    mon->eofCB(mon, mon->vm, failed);
+}
+
+
+
+
+
+qemuMonitorPtr
 qemuMonitorOpen(virDomainObjPtr vm,
-                int reconnect)
+                int reconnect,
+                qemuMonitorEOFNotify eofCB)
 {
+    qemuMonitorPtr mon;
+
+    if (VIR_ALLOC(mon) < 0) {
+        virReportOOMError(NULL);
+        return NULL;
+    }
+
+    mon->fd = -1;
+    mon->vm = vm;
+    mon->eofCB = eofCB;
+
     switch (vm->monitor_chr->type) {
     case VIR_DOMAIN_CHR_TYPE_UNIX:
-        return qemuMonitorOpenUnix(vm, vm->monitor_chr->data.nix.path,
+        mon->hasSendFD = 1;
-                                   reconnect);
+        mon->fd = qemuMonitorOpenUnix(vm, vm->monitor_chr->data.nix.path,
+                                      reconnect);
+        break;
+
     case VIR_DOMAIN_CHR_TYPE_PTY:
-        return qemuMonitorOpenPty(vm, vm->monitor_chr->data.file.path,
+        mon->fd = qemuMonitorOpenPty(vm, vm->monitor_chr->data.file.path,
-                                  reconnect);
+                                     reconnect);
+        break;
+
     default:
         qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
                          _("unable to handle monitor type: %s"),
                          virDomainChrTypeToString(vm->monitor_chr->type));
+        goto cleanup;
+    }
+
+    if ((mon->watch = virEventAddHandle(mon->fd,
+                                        VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR,
+                                        qemuMonitorIO,
+                                        mon, NULL)) < 0) {
+        qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s",
+                         _("unable to register monitor events"));
+        goto cleanup;
+    }
+
+    return mon;
+
+cleanup:
+    qemuMonitorClose(mon);
+    return NULL;
+}
+
+
+void qemuMonitorClose(qemuMonitorPtr mon)
+{
+    if (!mon)
+        return;
+
+    if (mon->watch)
+        virEventRemoveHandle(mon->watch);
+
+    if (mon->fd != -1)
+        close(mon->fd);
+    VIR_FREE(mon);
+}
+
+
+int qemuMonitorWrite(qemuMonitorPtr mon,
+                     const char *data,
+                     size_t len)
+{
+    return safewrite(mon->fd, data, len);
+}
+
+int qemuMonitorWriteWithFD(qemuMonitorPtr mon,
+                           const char *data,
+                           size_t len,
+                           int fd)
+{
+    struct msghdr msg;
+    struct iovec iov[1];
+    ssize_t ret;
+    char control[CMSG_SPACE(sizeof(int))];
+    struct cmsghdr *cmsg;
+
+    if (!mon->hasSendFD) {
+        errno = EINVAL;
         return -1;
     }
+
+    memset(&msg, 0, sizeof(msg));
+
+    iov[0].iov_base = (void *)data;
+    iov[0].iov_len = len;
+
+    msg.msg_iov = iov;
+    msg.msg_iovlen = 1;
+
+    msg.msg_control = control;
+    msg.msg_controllen = sizeof(control);
+
+    cmsg = CMSG_FIRSTHDR(&msg);
+    cmsg->cmsg_len = CMSG_LEN(sizeof(int));
+    cmsg->cmsg_level = SOL_SOCKET;
+    cmsg->cmsg_type = SCM_RIGHTS;
+    memcpy(CMSG_DATA(cmsg), &fd, sizeof(int));
+
+    do {
+        ret = sendmsg(mon->fd, &msg, 0);
+    } while (ret < 0 && errno == EINTR);
+
+    return ret == len ? 0 : -1;
+}
+
+int qemuMonitorRead(qemuMonitorPtr mon,
+                    char *data,
+                    size_t len)
+{
+    return read(mon->fd, data, len);
+}
+
+int qemuMonitorWaitForInput(qemuMonitorPtr mon)
+{
+    struct pollfd fd = { mon->fd, POLLIN | POLLERR | POLLHUP, 0 };
+
+retry:
+    if (poll(&fd, 1, -1) < 0) {
+        if (errno == EINTR)
+            goto retry;
+        return -1;
+    }
+    return 0;
 }

src/qemu/qemu_monitor.h

@@ -29,9 +29,33 @@
 
 #include "domain_conf.h"
 
-int qemuMonitorOpen(virDomainObjPtr vm,
+typedef struct _qemuMonitor qemuMonitor;
-                    int reconnect);
+typedef qemuMonitor *qemuMonitorPtr;
 
+typedef void (*qemuMonitorEOFNotify)(qemuMonitorPtr mon,
+                                     virDomainObjPtr vm,
+                                     int withError);
+
+qemuMonitorPtr qemuMonitorOpen(virDomainObjPtr vm,
+                               int reconnect,
+                               qemuMonitorEOFNotify eofCB);
+
+void qemuMonitorClose(qemuMonitorPtr mon);
+
+int qemuMonitorWrite(qemuMonitorPtr mon,
+                     const char *data,
+                     size_t len);
+
+int qemuMonitorWriteWithFD(qemuMonitorPtr mon,
+                           const char *data,
+                           size_t len,
+                           int fd);
+
+int qemuMonitorRead(qemuMonitorPtr mon,
+                    char *data,
+                    size_t len);
+
+int qemuMonitorWaitForInput(qemuMonitorPtr mon);
 
 
 #endif /* QEMU_MONITOR_H */

src/qemu/qemu_monitor_text.c

@@ -142,6 +142,7 @@ static char *qemuMonitorEscapeShell(const char *in)
  */
 static void
 qemuMonitorDiscardPendingData(virDomainObjPtr vm) {
+    qemuDomainObjPrivatePtr priv = vm->privateData;
     char buf[1024];
     int ret = 0;
 
@@ -149,54 +150,17 @@ qemuMonitorDiscardPendingData(virDomainObjPtr vm) {
      * get -1 or 0. Don't bother with detecting
      * errors, since we'll deal with that better later */
     do {
-        ret = read(vm->monitor, buf, sizeof (buf)-1);
+        ret = qemuMonitorRead(priv->mon, buf, sizeof (buf)-1);
     } while (ret > 0);
 }
 
-static int
-qemuMonitorSendUnix(const virDomainObjPtr vm,
-                    const char *cmd,
-                    size_t cmdlen,
-                    int scm_fd)
-{
-    struct msghdr msg;
-    struct iovec iov[1];
-    ssize_t ret;
-
-    memset(&msg, 0, sizeof(msg));
-
-    iov[0].iov_base = (void *)cmd;
-    iov[0].iov_len = cmdlen;
-
-    msg.msg_iov = iov;
-    msg.msg_iovlen = 1;
-
-    if (scm_fd != -1) {
-        char control[CMSG_SPACE(sizeof(int))];
-        struct cmsghdr *cmsg;
-
-        msg.msg_control = control;
-        msg.msg_controllen = sizeof(control);
-
-        cmsg = CMSG_FIRSTHDR(&msg);
-        cmsg->cmsg_len = CMSG_LEN(sizeof(int));
-        cmsg->cmsg_level = SOL_SOCKET;
-        cmsg->cmsg_type = SCM_RIGHTS;
-        memcpy(CMSG_DATA(cmsg), &scm_fd, sizeof(int));
-    }
-
-    do {
-        ret = sendmsg(vm->monitor, &msg, 0);
-    } while (ret < 0 && errno == EINTR);
-
-    return ret == cmdlen ? 0 : -1;
-}
 
 static int
 qemuMonitorSend(const virDomainObjPtr vm,
                 const char *cmd,
                 int scm_fd)
 {
+    qemuDomainObjPrivatePtr priv = vm->privateData;
     char *full;
     size_t len;
     int ret = -1;
@@ -208,20 +172,11 @@ qemuMonitorSend(const virDomainObjPtr vm,
 
     len = strlen(full);
 
-    switch (vm->monitor_chr->type) {
+    if (scm_fd == -1)
-    case VIR_DOMAIN_CHR_TYPE_UNIX:
+        ret = qemuMonitorWrite(priv->mon, full, len);
-        if (qemuMonitorSendUnix(vm, full, len, scm_fd) < 0)
+    else
-            goto out;
+        ret = qemuMonitorWriteWithFD(priv->mon, full, len, scm_fd);
-        break;
-    default:
-    case VIR_DOMAIN_CHR_TYPE_PTY:
-        if (safewrite(vm->monitor, full, len) != len)
-            goto out;
-        break;
-    }
 
-    ret = 0;
-out:
     VIR_FREE(full);
     return ret;
 }
@@ -234,12 +189,13 @@ qemuMonitorCommandWithHandler(const virDomainObjPtr vm,
                               void *handlerData,
                               int scm_fd,
                               char **reply) {
+    qemuDomainObjPrivatePtr priv = vm->privateData;
     int size = 0;
     char *buf = NULL;
 
     /* Should never happen, but just in case, protect
      * against null monitor (ocurrs when VM is inactive) */
-    if (!vm->monitor_chr)
+    if (!priv->mon)
         return -1;
 
     qemuMonitorDiscardPendingData(vm);
@@ -251,13 +207,10 @@ qemuMonitorCommandWithHandler(const virDomainObjPtr vm,
     *reply = NULL;
 
     for (;;) {
-        struct pollfd fd = { vm->monitor, POLLIN | POLLERR | POLLHUP, 0 };
-        char *tmp;
-
         /* Read all the data QEMU has sent thus far */
         for (;;) {
             char data[1024];
-            int got = read(vm->monitor, data, sizeof(data));
+            int got = qemuMonitorRead(priv->mon, data, sizeof(data));
 
             if (got == 0)
                 goto error;
@@ -279,6 +232,7 @@ qemuMonitorCommandWithHandler(const virDomainObjPtr vm,
         /* Look for QEMU prompt to indicate completion */
         if (buf) {
             char *foundPrompt;
+            char *tmp;
 
             if (extraPrompt &&
                 (foundPrompt = strstr(buf, extraPrompt)) != NULL) {
@@ -314,13 +268,10 @@ qemuMonitorCommandWithHandler(const virDomainObjPtr vm,
                 break;
             }
         }
-    pollagain:
+
         /* Need to wait for more data */
-        if (poll(&fd, 1, -1) < 0) {
+        if (qemuMonitorWaitForInput(priv->mon) < 0)
-            if (errno == EINTR)
-                goto pollagain;
             goto error;
-        }
     }
     *reply = buf;
     DEBUG("reply='%s'", buf);