[libvirt] [PATCH] qemu:Fix snapshot creating with vm xml persistent

[Peter Krempa]

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 at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180813/77323ea7/attachment-0001.sig>