On 10/26/2012 09:47 AM, Philipp Hahn wrote: > 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.
I think you read this backwards. This check says that if the domain is offline, it cannot be undefined, as that would strand the metadata. If the domain is active, then it is running, and all undefine does on a running domain is convert it to transient, but the domain is still running, so the metadata is still accessible (up until the transient domain halts, at which point it is cleaned up then). > 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. That shouldn't happen - virDomainUndefineFlags(0) on an inactive domain should fail if the domain has snapshots. Is this something you actually hit, and can you give me steps to reproduce? > >> + (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. The undefine is refused only if it would strand data. In the case of an active domain, the snapshots are not stranded because the now-transient domain still exists. > 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. I'm still not convinced we need your patch - using undefine on an active domain to convert it to transient is too soon to forcefully discard snapshots, as I could then redefine the domain to make it persistent again at which point the snapshots still make sense to keep around. -- Eric Blake ebl...@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
