[libvirt] [PATCHv3 08/43] snapshot: avoid crash when deleting qemu snapshots

[Daniel P. Berrange]

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 :|