On Mon, Oct 10, 2011 at 03:58:27PM -0600, Eric Blake wrote:
> 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.
yeah, that's what I'm worrying about mostly.
> 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.
Maybe we could 'just' add some test case in TCK or another test
suite trying to exhibit behaviour for some simpe case like parent
snapshot data gone missing or this kind of things.
> >
> > 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.
Yes that was my conclusion too after the patch set review, but you
have a far better view of the whole thing than me :-)
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list