[libvirt] [PATCHv2 4/3] snapshot: make offline qemu snapshots atomic

[Peter Krempa]

On 03/17/2012 05:33 PM, Eric Blake wrote:
> Offline internal snapshots can be rolled back with just a little
> bit of refactoring, meaning that we are now automatically atomic.
> 
> * src/qemu/qemu_domain.c (qemuDomainSnapshotForEachQcow2): Move
> guts...
> (qemuDomainSnapshotForEachQcow2Raw): ...to new helper, to allow
> rollbacks.
> * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Offline
> snapshots are now atomic.
> ---
You rollback changes to disks if other disks are not snapshotable, but later on, 
when the actual qemu-img command is run and fails the rollback is not performed. 

I's suggest squashing in:

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index a845480..8dde3c9 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1512,70 +1512,75 @@ qemuDomainSnapshotForEachQcow2Raw(struct qemud_driver *driver,

     for (i = 0; i < ndisks; i++) {
         /* FIXME: we also need to handle LVM here */
         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",
                              def->disks[i]->dst);
                     skipped = true;
                     continue;
                 } else if (STREQ(op, "-c") && i) {
                     /* We must roll back partial creation by deleting
                      * all earlier snapshots.  */
                     qemuDomainSnapshotForEachQcow2Raw(driver, def, name,
                                                       "-d", false, i);
                 }
                 qemuReportError(VIR_ERR_OPERATION_INVALID,
                                 _("Disk device '%s' does not support"
                                   " snapshotting"),
                                 def->disks[i]->dst);
                 return -1;
             }

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

             if (virRun(qemuimgarg, NULL) < 0) {
                 if (try_all) {
                     VIR_WARN("skipping snapshot action on %s",
                              def->disks[i]->dst);
                     skipped = true;
                     continue;
+                } else if (STREQ(op, "-c") && i) {
+                    /* We must roll back partial creation by deleting
+                     * all earlier snapshots.  */
+                    qemuDomainSnapshotForEachQcow2Raw(driver, def, name,
+                                                      "-d", false, i);
                 }
                 return -1;
             }
         }
     }

     return skipped ? 1 : 0;
 }


If you don't add this change the result is following:

Try to make a snapshot of a domain that has one of images missing:
virsh # snapshot-create-as Bare2 test1 test --atomic
error: internal error Child process (/usr/bin/qemu-img snapshot -c test1 /var/lib/libvirt/images/d2.qcow2) status unexpected: exit status 1

But the first image is still modified:
# qemu-img snapshot -l d.qcow2 
Snapshot list:
ID        TAG                 VM SIZE                DATE       VM CLOCK
1         test1                     0 2012-03-19 14:26:50   00:00:00.000


Otherwise looks good. ACK with that suggested change.

Peter