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

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org