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

Re: [libvirt] [PATCH 1.5/2] snapshot: use qemu-img on disks in use at time of snapshot



On 10/04/2011 07:39 PM, Eric Blake wrote:
Once we know which set of disks belong to a snapshot, reverting or
deleting that snapshot should visit just those disks, rather than
also visiting disks that were hot-plugged in the meantime or
skipping disks that were hot-unplugged in the meantime.

* src/qemu/qemu_domain.c (qemuDomainSnapshotForEachQcow2): Use
snapshot domain details when available.  Avoid NULL deref.
---

Detected while actually testing patch 2/2 in the series.  This
fixes the issue, but is worth a separate review before I push
the series.

  src/qemu/qemu_domain.c |   24 ++++++++++++++++--------
  1 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 65f721a..85bebd6 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1402,6 +1402,14 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver *driver,
      const char *qemuimgarg[] = { NULL, "snapshot", NULL, NULL, NULL, NULL };
      int i;
      bool skipped = false;
+    virDomainDefPtr def;
+
+    /* Prefer action on the disks in use at the time the snapshot was
+     * created; but fall back to current definition if dealing with a
+     * snapshot created prior to libvirt 0.9.5.  */
+    def = snap->def->dom;
+    if (!def)
+        def = vm->def;

      qemuimgarg[0] = qemuFindQemuImgBinary(driver);
      if (qemuimgarg[0] == NULL) {
@@ -1412,36 +1420,36 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver *driver,
      qemuimgarg[2] = op;
      qemuimgarg[3] = snap->def->name;

-    for (i = 0; i<  vm->def->ndisks; i++) {
+    for (i = 0; i<  def->ndisks; i++) {
          /* FIXME: we also need to handle LVM here */
          /* FIXME: if we fail halfway through this loop, we are in an
           * inconsistent state.  I'm not quite sure what to do about that
           */
-        if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
-            if (!vm->def->disks[i]->driverType ||
-                STRNEQ(vm->def->disks[i]->driverType, "qcow2")) {
+        if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+            if (!def->disks[i]->driverType ||
+                STRNEQ(def->disks[i]->driverType, "qcow2")) {
                  if (try_all) {
                      /* Continue on even in the face of error, since other
                       * disks in this VM may have the same snapshot name.
                       */
                      VIR_WARN("skipping snapshot action on %s",
-                             vm->def->disks[i]->dst);
+                             def->disks[i]->dst);
                      skipped = true;
                      continue;
                  }
                  qemuReportError(VIR_ERR_OPERATION_INVALID,
                                  _("Disk device '%s' does not support"
                                    " snapshotting"),
-                                vm->def->disks[i]->dst);
+                                def->disks[i]->dst);
                  return -1;
              }

-            qemuimgarg[4] = vm->def->disks[i]->src;
+            qemuimgarg[4] = def->disks[i]->src;

              if (virRun(qemuimgarg, NULL)<  0) {
                  if (try_all) {
                      VIR_WARN("skipping snapshot action on %s",
-                             vm->def->disks[i]->info.alias);
+                             def->disks[i]->dst);
                      skipped = true;
                      continue;
                  }

ACK.


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