[PATCH 1/3] Fix up qemu domain save/managed save locking.

[Chris Lalancette <clalance@xxxxxxxxxx>]

The current version of the qemu managed save implementation
is subject to a race where the domain shuts down between
the time that we start the command and the time that we
actually try to do the save.  Close this race by making
qemuDomainSaveFlags() expect both the driver and the passed-in
vm object to be locked before executing.

Signed-off-by: Chris Lalancette <clalance@xxxxxxxxxx>
---
 src/qemu/qemu_driver.c |   79 ++++++++++++++++++-----------------------------
 1 files changed, 30 insertions(+), 49 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 31cf1fe..b72aace 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5213,12 +5213,11 @@ endjob:
     return ret;
 }
 
-
-static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
+/* this internal function expects the driver lock to already be held on entry */
+static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom,
+                               virDomainObjPtr vm, const char *path,
                                int compressed)
 {
-    struct qemud_driver *driver = dom->conn->privateData;
-    virDomainObjPtr vm = NULL;
     char *xml = NULL;
     struct qemud_save_header header;
     struct fileOpHookData hdata;
@@ -5236,19 +5235,8 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
     memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic));
     header.version = QEMUD_SAVE_VERSION;
 
-    qemuDriverLock(driver);
-
     header.compressed = compressed;
 
-    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
-
-    if (!vm) {
-        char uuidstr[VIR_UUID_STRING_BUFLEN];
-        virUUIDFormat(dom->uuid, uuidstr);
-        qemuReportError(VIR_ERR_NO_DOMAIN,
-                        _("no domain with matching uuid '%s'"), uuidstr);
-        goto cleanup;
-    }
     priv = vm->privateData;
 
     if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
@@ -5533,12 +5521,9 @@ cleanup:
     VIR_FREE(xml);
     if (ret != 0 && is_reg)
         unlink(path);
-    if (vm)
-        virDomainObjUnlock(vm);
     if (event)
         qemuDomainEventQueue(driver, event);
     virCgroupFree(&cgroup);
-    qemuDriverUnlock(driver);
     return ret;
 }
 
@@ -5546,8 +5531,11 @@ static int qemudDomainSave(virDomainPtr dom, const char *path)
 {
     struct qemud_driver *driver = dom->conn->privateData;
     int compressed;
+    int ret = -1;
+    virDomainObjPtr vm = NULL;
+
+    qemuDriverLock(driver);
 
-    /* Hm, is this safe against qemudReload? */
     if (driver->saveImageFormat == NULL)
         compressed = QEMUD_SAVE_FORMAT_RAW;
     else {
@@ -5560,7 +5548,23 @@ static int qemudDomainSave(virDomainPtr dom, const char *path)
         }
     }
 
-    return qemudDomainSaveFlag(dom, path, compressed);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    if (!vm) {
+        char uuidstr[VIR_UUID_STRING_BUFLEN];
+        virUUIDFormat(dom->uuid, uuidstr);
+        qemuReportError(VIR_ERR_NO_DOMAIN,
+                        _("no domain with matching uuid '%s'"), uuidstr);
+        goto cleanup;
+    }
+
+    ret = qemudDomainSaveFlag(driver, dom, vm, path, compressed);
+
+cleanup:
+    if (vm)
+        virDomainObjUnlock(vm);
+    qemuDriverUnlock(driver);
+
+    return ret;
 }
 
 static char *
@@ -5593,48 +5597,25 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags)
         virUUIDFormat(dom->uuid, uuidstr);
         qemuReportError(VIR_ERR_NO_DOMAIN,
                         _("no domain with matching uuid '%s'"), uuidstr);
-        goto error;
-    }
-
-    if (!virDomainObjIsActive(vm)) {
-        qemuReportError(VIR_ERR_OPERATION_INVALID,
-                        "%s", _("domain is not running"));
-        goto error;
+        goto cleanup;
     }
 
     name = qemuDomainManagedSavePath(driver, vm);
     if (name == NULL)
-        goto error;
+        goto cleanup;
 
     VIR_DEBUG("Saving state to %s", name);
 
-    /* FIXME: we should take the flags parameter, and use bits out
-     * of there to control whether we are compressing or not
-     */
     compressed = QEMUD_SAVE_FORMAT_RAW;
-
-    /* we have to drop our locks here because qemudDomainSaveFlag is
-     * going to pick them back up.  Unfortunately it opens a race window
-     * between us dropping and qemudDomainSaveFlag picking it back up, but
-     * if we want to allow other operations to be able to happen while
-     * qemuDomainSaveFlag is running (possibly for a long time), I'm not
-     * sure if there is a better solution
-     */
-    virDomainObjUnlock(vm);
-    qemuDriverUnlock(driver);
-
-    ret = qemudDomainSaveFlag(dom, name, compressed);
+    ret = qemudDomainSaveFlag(driver, dom, vm, name, compressed);
 
 cleanup:
-    VIR_FREE(name);
-
-    return ret;
-
-error:
     if (vm)
         virDomainObjUnlock(vm);
     qemuDriverUnlock(driver);
-    goto cleanup;
+    VIR_FREE(name);
+
+    return ret;
 }
 
 static int
-- 
1.7.2.1

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list