Hello Eric,
On Friday 02 September 2011 06:25:01 Eric Blake wrote:
Just as leaving managed save metadata behind can cause problems
when creating a new domain that happens to collide with the name
of the just-deleted domain, the same is true of leaving any
snapshot metadata behind.
I just noticed a problem with that patch
282fe1f08c89189e36142fc2d12bae0175038bdd:
index ac71b04..38a21db 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4852,6 +4853,14 @@ qemuDomainUndefineFlags(virDomainPtr dom,
goto cleanup;
}
+ if (!virDomainObjIsActive(vm) &&
This check restricts the test to only running domains, that is if you undefine
a currently inactive domain, its snapshot metadata is left behind while the
domain is deleted.
This behaviour is actually documented in
<
http://libvirt.org/html/libvirt-libvirt.html#virDomainUndefineFlags> (now
that I know what I have to look at), but I was still surprised that
virDomainUndefineFlags(0) returned success on my inactive domain with
snapshots.
+ (nsnapshots =
virDomainSnapshotObjListNum(&vm->snapshots, 0))) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ _("cannot delete inactive domain with %d
snapshots"), + nsnapshots);
+ goto cleanup;
+ }
+
if (!vm->persistent) {
qemuReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("cannot undefine transient
domain"));
Is this restriction to only delete the snapshots for active domains on purpose
or am I missing something?
I would expect the undefine to be refused for all states, not just for active
domains.
I would prefer the snapshots to be deleted for both active and inactive
domains, since qemuDomainSnapshotDiscardAllMetadata() is not available
externally. And iterating over all snapshots to just delete them seems to be
wasteful, especially when you use qcow2 with its reference counting issues.
See the attached patch for my proposal.
Sincerely
Philipp
--
Philipp Hahn Open Source Software Engineer hahn(a)univention.de
Univention GmbH be open. fon: +49 421 22 232- 0
Mary-Somerville-Str.1 D-28359 Bremen fax: +49 421 22 232-99
http://www.univention.de/