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@didichuxing.com
---
 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);
-- 
1.8.3.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@didichuxing.com
> ---
>  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
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.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@didichuxing.com
> > ---
> >  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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list