On 28.05.2019 15:55, Peter Krempa wrote: > On Tue, May 28, 2019 at 14:34:18 +0300, Nikolay Shirokovskiy wrote: >> The function updates vm->newDef and frees previous pesistent definition >> which can be used by in different thread by some API that unlocks domain >> to communicate to qemu. So we have an access to freed memory in that >> thread. Let's acquire job condition when changing persistent xml. >> >> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx> >> --- >> >> A bug example (some downstream version of libvirt, nevertheless): >> >> (gdb) bt >> #0 0x00007f3bb77a0533 in virDomainDefHasVcpusOffline (def=def@entry=0x7f3b7802a830) at conf/domain_conf.c:1637 >> #1 0x00007f3bb77cb32f in virDomainCpuDefFormat (def=0x7f3b7802a830, buf=0x7f3ba7308940) at conf/domain_conf.c:27503 >> #2 virDomainDefFormatInternal (def=def@entry=0x7f3b7802a830, caps=0x7f3b70000ae0, flags=3, flags@entry=1, >> buf=buf@entry=0x7f3ba7308940, xmlopt=xmlopt@entry=0x0) at conf/domain_conf.c:27850 >> #3 0x00007f3bb77ced46 in virDomainDefFormat (def=def@entry=0x7f3b7802a830, caps=<optimized out>, >> flags=flags@entry=1) at conf/domain_conf.c:28564 >> #4 0x00007f3bb77d23f9 in virDomainSaveConfig (configDir=0x7f3b981227e0 "/etc/libvirt/qemu", caps=<optimized out>, >> def=0x7f3b7802a830) at conf/domain_conf.c:28776 >> #5 0x00007f3ba086b617 in qemuDomainSetMemoryStatsPeriod (dom=<optimized out>, period=30, flags=<optimized out>) >> at qemu/qemu_driver.c:4839 >> #6 0x00007f3bb7905613 in virDomainSetMemoryStatsPeriod (domain=domain@entry=0x7f3b74006200, period=30, flags=3) >> at libvirt-domain.c:2087 >> >> (gdb) f 5 >> #5 0x00007f3ba086b617 in qemuDomainSetMemoryStatsPeriod (dom=<optimized out>, period=30, flags=<optimized out>) >> at qemu/qemu_driver.c:4839 >> 4839 ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef); >> >> (gdb) p vm->def >> $8 = (virDomainDefPtr) 0x7f3b8400c6f0 >> (gdb) p vm->newDef >> $9 = (virDomainDefPtr) 0x7f3b7001de20 >> (gdb) p persistentDef >> $10 = (virDomainDefPtr) 0x7f3b7802a830 >> >> src/qemu/qemu_driver.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index 42b1ce2..683dcaa 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -7626,6 +7626,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, >> virCapsPtr caps = NULL; >> unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | >> VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; >> + bool job = false; >> >> virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); >> >> @@ -7647,6 +7648,10 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, >> if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) >> goto cleanup; >> >> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) >> + goto cleanup; >> + job = true; > > How is this supposed to work? 'vm' is NULL ... > >> + >> if (!(vm = virDomainObjListAdd(driver->domains, def, > > ... until this assignment. > >> driver->xmlopt, >> 0, &oldDef))) Yeah, this was a bit :) dumb Nikolay -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
