Re: [libvirt] [PATCH 5/4] qemu: properly revert to offline snapshots

On 08/10/2011 12:24 AM, Philipp Hahn wrote: Sure. 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(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org