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

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.