On 08/10/2011 12:24 AM, Philipp Hahn wrote:
Hello Eric,
you're patch looks very similar to mine, which I created myself yesterday, but
hadn't had time to actually send after doing the testing. I'll attach it just
FYI.
On Wednesday 10 August 2011 01:28:42 Eric Blake wrote:
qemuDomainSnapshotRevertInactive has the same FIXMEs as
qemuDomainSnapshotCreateInactive, so algorithmic fixes to properly
handle partial loop iterations should be applied later to both
functions, but we're not making the situation any worse in
this patch.
* src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Use
qemu-img rather than 'qemu -loadvm' to revert to offline snapshot.
(qemuDomainSnapshotRevertInactive): New helper.
s/offline/inactive/ just for consistency.
Sure.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
...
@@ -8846,7 +8893,7 @@ static int
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
}
- if (qemuDomainSnapshotSetCurrentActive(vm, driver->snapshotDir)< 0)
+ if (qemuDomainSnapshotRevertInactive(vm, snap)< 0)
Now you don't call qemuDomainSnapshotSetCurrent() any more, which marks the
snapshot at being current (what ever that is important for).
That's why I in my patch also have to change qemuBuildCommandLine() to not
add -loadvm, since qemu then will refuse to start.
Right now, qemu's _only_ use of <active> (which is set to non-zero by
qemuDomainSnapshotSetCurrentActive) is to tell qemuBuildCommandLine to
use -loadvm to revert to that point, rather than to do a fresh boot.
Your approach of keeping the call means that qemuBuildCommandLine has to
special-case to not use -loadvm for inactive snapshots; whereas my
approach of no longer marking the snapshot active for inactive snapshots
means that the existing qemuBuildCommandLine no longer sees an active
snapshot in the first place; both approaches end up omitting the -loadvm
call on the next domain boot, but mine does it with less code and
without using <active>.
Meanwhile, I have plans to repurpose the <active> element. Right now,
it is stored as a long, but only ever contains the value 0 or 1 as used
by qemu (it is not used by other drivers), and is used by at most one
snapshot at a time. But since we _want_ to fix the bug where libvirtd
forgets the current snapshot when it is restarted, I propose that active
be repurposed slightly, as well as changed to int or even bool, since
long is overkill:
qemuBuildCommandLine is already being passed current_snapshot or NULL;
that should be sufficient to decide whether to add the -loadvm argument
without also probing active; so all callers should be updated to pass
NULL except for reverting from an active snapshot when qemu is not
already running - this means a signature tweak to qemuProcessStart.
Once that is done, qemu is no longer using active for its own purposes,
so conf/domain_conf can then take over <active> to uniformly identify
which snapshot is current as part of the snapshot xml (that is, all but
one snapshot will be <active>0</active>, and the current snapshot will
be <active>1</active>). Every time the current snapshot changes, this
implies updating as many as two files in the snapshot xml directory.
<active> is already an internal-only xml element - domain_conf ignores
it on user input, and strips it on dumpxml output visible to the user,
so it is only present in the internal xml files. The idea is that if
there is at least one snapshot, then there generally be a current
snapshot; the only time that there is not a current snapshot but where
snapshots still exist is when you create multiple trees in the hierarchy
by setting the current snapshot to the root of the hierarchy, then
delete that snapshot while keeping its children intact (making each
child an independent root).
More patches coming later today along these lines, once I test it all.
Another goal of mine is to introduce 'virsh snapshot-list dom --tree',
which shows snapshot names with a visual hierarchy relationship:
root
child1
grandchild1.1
grandchild1.2
child2
grandchild2.1
grandchild2.2
That would be made much easier if we had an API for
virDomainSnapshotGetParentName (see also my wish for
virDomainSnapshotGetName in patch 6/4); but it can also be done with xml
scraping using existing API.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list