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

Re: [libvirt] [PATCH 2/2] snapshot: wire up qemu transaction command



On 03/17/2012 10:27 PM, Eric Blake wrote:
The hardest part about adding transactions is not using the new
monitor command, but undoing the partial changes we made prior
to a failed transaction.

* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive): Use
transaction when available.
(qemuDomainSnapshotUndoSingleDiskActive): New function.
(qemuDomainSnapshotCreateSingleDiskActive): Pass through actions.
(qemuDomainSnapshotCreateXML): Adjust caller.
---
  src/qemu/qemu_driver.c |  112 +++++++++++++++++++++++++++++++++++++++++++++--
  1 files changed, 107 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c3bbc3f..2bc8d5d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9822,7 +9822,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver,
                                           virDomainObjPtr vm,
                                           virDomainSnapshotDiskDefPtr snap,
                                           virDomainDiskDefPtr disk,
-                                         virDomainDiskDefPtr persistDisk)
+                                         virDomainDiskDefPtr persistDisk,
+                                         virJSONValuePtr actions)
  {
      qemuDomainObjPrivatePtr priv = vm->privateData;
      char *device = NULL;
@@ -9882,7 +9883,7 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver,
      origdriver = NULL;

      /* create the actual snapshot */
-    ret = qemuMonitorDiskSnapshot(priv->mon, NULL, device, source);
+    ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source);
      virDomainAuditDisk(vm, disk->src, source, "snapshot", ret>= 0);
      if (ret<  0)
          goto cleanup;
@@ -9923,6 +9924,69 @@ cleanup:
      return ret;
  }

+/* The domain is expected to hold monitor lock.  This is the
+ * counterpart to qemuDomainSnapshotCreateSingleDiskActive, called
+ * only on a failed transaction. */
+static void
+qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver *driver,
+                                       virDomainObjPtr vm,
+                                       virDomainDiskDefPtr origdisk,
+                                       virDomainDiskDefPtr disk,
+                                       virDomainDiskDefPtr persistDisk,
+                                       bool need_unlink)
+{
+    char *source = NULL;
+    char *driverType = NULL;
+    char *persistSource = NULL;
+    char *persistDriverType = NULL;
+    struct stat st;
+
+    if (!(source = strdup(origdisk->src)) ||
+        (origdisk->driverType&&
+         !(driverType = strdup(origdisk->driverType))) ||
+        (persistDisk&&
+         (!(persistSource = strdup(source)) ||
+          (driverType&&  !(persistDriverType = strdup(driverType)))))) {
+        virReportOOMError();
+        goto cleanup;
+    }
+
+    if (virSecurityManagerRestoreImageLabel(driver->securityManager,
+                                            vm->def, disk)<  0)
+        VIR_WARN("Unable to restore security label on %s", disk->src);
+    if (virDomainLockDiskDetach(driver->lockManager, vm, disk)<  0)
+        VIR_WARN("Unable to release lock on %s", source);
+    if (need_unlink&&  stat(disk->src,&st) == 0
+&&  st.st_size == 0&&  S_ISREG(st.st_mode)&&  unlink(disk->src)<  0)
+        VIR_WARN("Unable to release lock on %s", disk->src);

Is this second warning correct? It looks to me like if you are unlinking the disk source file, but the error message states that lock release failed.

+
+    /* Update vm in place to match changes.  */
+    VIR_FREE(disk->src);
+    disk->src = source;
+    source = NULL;
+    VIR_FREE(disk->driverType);
+    if (driverType) {
+        disk->driverType = driverType;
+        driverType = NULL;
+    }
+    if (persistDisk) {
+        VIR_FREE(persistDisk->src);
+        persistDisk->src = persistSource;
+        persistSource = NULL;
+        VIR_FREE(persistDisk->driverType);
+        if (persistDriverType) {
+            persistDisk->driverType = persistDriverType;
+            persistDriverType = NULL;
+        }
+    }
+
+cleanup:
+    VIR_FREE(source);
+    VIR_FREE(driverType);
+    VIR_FREE(persistSource);
+    VIR_FREE(persistDriverType);
+}
+

I don't have many experience working with the locking and security code, but this funcion looks as it's doing what it should. I'd feel more confident if somebody other would look at this part.

ACK to the rest, and a not-so-confident ACK of the marked part if my question gets cleared.

Peter


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