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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list