On Wed, Aug 24, 2011 at 09:22:25AM -0600, Eric Blake wrote:
This one's nasty. Ever since we fixed virHashForEach to prevent nested hash iterations for safety reasons, virDomainSnapshotDelete with VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN has been broken for qemu: it deletes children, while leaving grandchildren intact but pointing to a no-longer-present parent. But even before then, the code would often appear to succeed to clean up grandchildren, but risked memory corruption if you have a large and deep hierarchy of snapshots.
For acting on just children, a single virHashForEach is sufficient. But for acting on an entire subtree, it requires iteration; and since we declared recursion as invalid, we have to switch to a while loop. Doing this correctly requires quite a bit of overhaul, so I added a new helper function to isolate the algorithm from the actions, so that callers do not have to reinvent the iteration.
* src/conf/domain_conf.h (_virDomainSnapshotDef): Add mark. (virDomainSnapshotForEachDescendant): New prototype. * src/libvirt_private.syms (domain_conf.h): Export it. * src/conf/domain_conf.c (virDomainSnapshotMarkDescendant) (virDomainSnapshotActOnDescendant) (virDomainSnapshotForEachDescendant): New functions. * src/qemu/qemu_driver.c (qemuDomainSnapshotDiscardChildren): Replace... (qemuDomainSnapshotDiscardDescenent): ...with callback that doesn't nest hash traversal. (qemuDomainSnapshotDelete): Use new function. --- src/conf/domain_conf.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 8 +++- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 35 ++++++---------- 4 files changed, 124 insertions(+), 23 deletions(-)
Perhaps I'm mis-understanding what this patch is attempting to fix, but after applying everything upto this point, I see the following behaviour which does not appear correct $ virsh list Id Name State ---------------------------------- 5 vm1 running $ virsh snapshot-list vm1 Name Creation Time State --------------------------------------------------- $ virsh snapshot-create vm1 Domain snapshot 1314203761 created $ virsh snapshot-create vm1 Domain snapshot 1314203763 created $ virsh snapshot-create vm1 Domain snapshot 1314203767 created $ virsh snapshot-create vm1 Domain snapshot 1314203777 created $ virsh snapshot-create vm1 Domain snapshot 1314203781 created $ virsh snapshot-list vm1 Name Creation Time State --------------------------------------------------- 1314203761 2011-08-24 17:36:01 +0100 running 1314203763 2011-08-24 17:36:03 +0100 running 1314203767 2011-08-24 17:36:07 +0100 running 1314203777 2011-08-24 17:36:17 +0100 running 1314203781 2011-08-24 17:36:21 +0100 running $ virsh snapshot-delete vm1 --children 1314203767 Domain snapshot 1314203767 deleted $ virsh snapshot-list vm1 Name Creation Time State --------------------------------------------------- 1314203761 2011-08-24 17:36:01 +0100 running 1314203763 2011-08-24 17:36:03 +0100 running 1314203781 2011-08-24 17:36:21 +0100 running IIUC, 1314203781 should have been deleted. I also still saw errors in libvirtd logs 17:36:36.175: 4423: info : libvirt version: 0.9.4 17:36:36.175: 4423: error : virHashSearch:604 : Hash operation not allowed during iteration 17:36:36.176: 4423: warning : virDomainSnapshotMarkDescendant:11309 : snapshot hash table is inconsistent! 17:36:36.176: 4423: error : virHashSearch:604 : Hash operation not allowed during iteration 17:36:36.176: 4423: warning : virDomainSnapshotMarkDescendant:11309 : snapshot hash table is inconsistent! 17:36:36.176: 4423: error : virHashSearch:604 : Hash operation not allowed during iteration 17:36:36.176: 4423: warning : virDomainSnapshotMarkDescendant:11309 : snapshot hash table is inconsistent! Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|