On Thu, Aug 09, 2018 at 17:01:49 +0200, Michal Privoznik wrote: > On 08/07/2018 01:46 PM, Bobo Du wrote: > > the vm xml will be existed when the vm is undefined after started. > > blockcommit interface also has the bug with above. > > Step1:prepare a vm，eg:test1,start it and undefined > > Step2: virsh snapshot-create-as test1 --disk-only --no-metadata > > Step3:ls /etc/libvirt/qemu/test1.xml,then it will be exist here > > > > Signed-off-by: Bobo Du <dubobo@xxxxxxxxxxxxxxx > > --- > > src/qemu/qemu_driver.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > > index fb0d4a8..d977922 100644 > > --- a/src/qemu/qemu_driver.c > > +++ b/src/qemu/qemu_driver.c > > @@ -7570,6 +7570,7 @@ qemuDomainUndefineFlags(virDomainPtr dom, > > if (!virDomainObjIsActive(vm)) > > qemuDomainRemoveInactive(driver, vm); > > > > + virDomainDefFree(vm->newDef); > > ret = 0; > > endjob: > > qemuDomainObjEndJob(driver, vm); > > > > This doesn't feel right. Firstly, vm->newDef becomes a stale pointer This is obviously true ... > after this patch. But more importantly, if snapshot-create-as saves the > xml it is clearly not testing vm->persistent flag and the fix should > focus on that. That was the previous approach which is wrong in my opinion. If vm->newDef exists we should modify it. The snapshot code should not care if the VM is persistent or not. We need to modify the definition and afterwards it's saved. If the vm->newDef gets used in a different way we'd need to change a lot of code just because of that. The real bug is that for transient vms currently there should be no vm->newDef as this is a fact in many places. Also the commit message is obviously misleading as it's pointing out the symptom but not the real bug. If vm->newDef gets cleared and the commit message points out to the real problem this should be the preferred solution.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
