Re: [libvirt] [PATCHv2 4/3] snapshot: make offline qemu snapshots atomic
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