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.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org