[libvirt] [PATCH 2/4] snapshot: track qemu snapshot relations

Eric Blake eblake at redhat.com
Mon Oct 10 21:58:27 UTC 2011


On 10/09/2011 08:43 PM, Daniel Veillard wrote:
> On Fri, Oct 07, 2011 at 06:05:55PM -0600, Eric Blake wrote:
>> Maintain the parent/child relationships of all qemu snapshots.
>>
>> * src/qemu/qemu_driver.c (qemuDomainSnapshotLoad): Populate
>> relationships after loading.
>> (qemuDomainSnapshotCreateXML): Set relations on creation; tweak
>> redefinition to reuse existing object.
>> (qemuDomainSnapshotReparentChildren, qemuDomainSnapshotDelete):
>> Clear relations on delete.
>> ---
>>   src/qemu/qemu_driver.c |   72 ++++++++++++++++++++++++++++++++++++++---------
>>   1 files changed, 58 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 5db67b8..501a3fc 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -376,6 +376,10 @@ static void qemuDomainSnapshotLoad(void *payload,
>>           vm->current_snapshot = NULL;
>>       }
>>
>> +    if (virDomainSnapshotUpdateRelations(&vm->snapshots)<  0)
>> +        VIR_ERROR(_("Snapshots have inconsistent relations for domain %s"),
>> +                  vm->def->name);
>> +
>
>   Hum, so we error out but continue with possibly inconsistent metadata,
> isn't that risky ? What would be the cost or failing the load here and
> consequences of lack of metadata ?

With just patch 2/4, the consequence of ignoring inconsistent metadata 
is that things fall back to searching the entire hash table, which will 
repeat the inconsistent checks, but possibly infloop (it's hard to say 
for sure, because it's quite a bit of code to audit).  With patch 3/4 
added, ignoring inconsistent metadata will mean that all consistent 
snapshots will still be visited, while the inconsistent ones can still 
be looked up by name but cannot be traversed (they act as if they have 
no parent or children).  I did prove to myself that using _only_ libvirt 
APIs to modify the hierarchy, then the metadata will always be consistent.

I think (but did not prove) that the code will not segv, but 
inconsistent data still has the possibility of triggering an infloop. 
On the other hand, the only way to get inconsistent metadata is to 
manually modify files under /etc/libvirt or /var/run/libvirt, which we 
already discourage, so I'm not too worried - if someone can manage to 
get libvirtd hung on an infloop by going behind libvirt's back, that's 
their fault, not libvirt's.

But if you are worried, I could go one step further and refuse to load 
the snapshot hierarchy altogether if inconsistent data was found in any 
of the snapshot xml files, and make commands like virDomainSnapshotNum 
return failure when it detected that snapshot metadata could not be loaded.

>
>    Okay, the main problem is making sure we keep the metadata on all
>    operations that are needed, Load/Create/Delete and Reparent sounds
>    right,

Yes, I double-checked, and all code that registered or removed a 
snapshot, or which modified the metadata file of a snapshot, takes care 
to update the new metadata fields correctly.

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




More information about the libvir-list mailing list