Re: [libvirt] [PATCH] qemu:Fix snapshot creating with vm xml persistent
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 > --- > > 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. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu:Fix snapshot creating with vm xml persistent
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 --- > 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
[libvirt] [PATCH] qemu:Fix snapshot creating with vm xml persistent
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 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
Re: [libvirt] [PATCH] qemu:Fix snapshot creating with vm xml persistent
On Thu, Jul 26, 2018 at 03:03:04 -0400, Bobo Du wrote: > it is a bug when the vm is undefined after started. > blockcommit interface also has the bug with above. Could you please specify steps and the outcome when this breaks? vm->newDef should not be present if the VM is not persistent so the code below should be correct as is now. I think there might be some other bug. > > Signed-off-by: Bobo Du --- > src/qemu/qemu_blockjob.c | 2 +- > src/qemu/qemu_driver.c | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu:Fix snapshot creating with vm xml persistent
it is a bug when the vm is undefined after started.
blockcommit interface also has the bug with above.
Signed-off-by: Bobo Du xmlopt, cfg->stateDir, vm, driver->caps) <
0)
VIR_WARN("Unable to save status on vm %s after block job",
vm->def->name);
-if (status == VIR_DOMAIN_BLOCK_JOB_COMPLETED && vm->newDef) {
+if (status == VIR_DOMAIN_BLOCK_JOB_COMPLETED && vm->newDef &&
vm->persistent) {
if (virDomainSaveConfig(cfg->configDir, driver->caps, vm->newDef) < 0)
VIR_WARN("Unable to update persistent definition on vm %s "
"after block job", vm->def->name);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fb0d4a8..202c1ba 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15055,7 +15055,8 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr
driver,
if (ret == 0 || !do_transaction) {
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm,
driver->caps) < 0 ||
-(persist && virDomainSaveConfig(cfg->configDir, driver->caps,
+(vm->persistent && persist &&
virDomainSaveConfig(cfg->configDir,
+driver->caps,
vm->newDef) < 0))
ret = -1;
}
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu:Fix snapshot creating with vm xml persistent
Bobo Du (1): qemu:Fix snapshot creating with vm xml persistent src/qemu/qemu_blockjob.c | 2 +- src/qemu/qemu_driver.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
