[libvirt] [PATCH 2/2] snapshot: enforce REVERT_FORCE on qemu

[Eric Blake]

On 10/04/2011 01:26 PM, Laine Stump wrote:
> On 09/30/2011 02:52 PM, Eric Blake wrote:
>> Implements the documentation for snapshot revert vs. force.
>>
>> Part of the patch tightens existing behavior (previously, reverting
>> to an old snapshot without<domain> was blindly attempted, now it
>> requires force), while part of it relaxes behavior (previously, it
>> was not possible to revert an active domain to an ABI-incompatible
>> active snapshot, now force allows this transition).
>>
>> * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Check for
>> risky situations, and allow force to get past them.
>
> ACK.

Before pushing this, I'm running some sanity tests.  So far, this test 
sequence (adjusted to the fixed code) shows where force helps with older 
snapshots (I'll send separate email for showing how force helps active 
ABI-incompatible snapshots):

Test 1:
$ virsh snapshot-create-as dom snap # offline domain with one qcow2 disk
$ virsh edit dom # add a second qcow2 disk
$ virsh snapshot-revert dom snap # offline revert doesn't need force
$ virsh dumpxml dom # sure enough, second disk is gone
$ virsh edit dom # add a second qcow2 disk
$ virsh snapshot-dumpxml dom snap > snap.xml
$ virsh snapshot-delete --metadata dom snap
$ sed -i '\|<domain |,\|</domain>| d' snap.xml
$ virsh snapshot-create --redefine fedora_15-32 snap.xml
# the delete --metadata/--redefine is necessary, so that libvirt
# won't reuse <domain> from the prior definition
$ virsh snapshot-revert dom snap # simulates reverting to 0.9.4 snapshot
error: revert requires force: snapshot 'snap' lacks domain 'dom' 
rollback info
$ virsh snapshot-revert dom snap --force
error: internal error Child process (/usr/bin/qemu-img snapshot -a snap 
/path/to/second.qcow2) status unexpected: exit status 1
# See why we shouldn't have allowed blind revert? :)
$ virsh edit dom # remove that second disk
$ virsh snapshot-revert dom snap --force
# now that we match expected state, the revert works as desired

And here's what I have to squash in for test 1 to succeed as planned.

diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index 1a171cf..07c4fd4 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -9649,7 +9649,8 @@ static int 
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
      virDomainDefPtr config = NULL;

      virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
-                  VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED, -1);
+                  VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED |
+                  VIR_DOMAIN_SNAPSHOT_REVERT_FORCE, -1);

      /* We have the following transitions, which create the following 
events:
       * 1. inactive -> inactive: none
@@ -9701,8 +9702,8 @@ static int 
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
      if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) {
          if (!snap->def->dom) {
              qemuReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY,
-                            _("snapshot lacks domain '%s' rollback 
details"),
-                            snap->def->name);
+                            _("snapshot '%s' lacks domain '%s' rollback 
info"),
+                            snap->def->name, vm->def->name);
              goto cleanup;
          }
          if (virDomainObjIsActive(vm) &&
diff --git i/src/qemu/qemu_domain.c w/src/qemu/qemu_domain.c
index 65f721a..dda53f3 100644
--- i/src/qemu/qemu_domain.c
+++ w/src/qemu/qemu_domain.c
@@ -1441,7 +1441,7 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver 
*driver,
              if (virRun(qemuimgarg, NULL) < 0) {
                  if (try_all) {
                      VIR_WARN("skipping snapshot action on %s",
-                             vm->def->disks[i]->info.alias);
+                             vm->def->disks[i]->dst);
                      skipped = true;
                      continue;
                  }


-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org