Re: [libvirt] [PATCH] qemu: aquire job in qemuDomainDefineXMLFlags

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(a)virtuozzo.com&gt; > --- > > 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)))