Re: [libvirt] [PATCHv3 08/43] snapshot: avoid crash when deleting qemu snapshots
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 :|