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

[Nikolay Shirokovskiy]

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 at virtuozzo.com>
>> ---
>>
>> A bug example (some downstream version of libvirt, nevertheless):
>>
>>     (gdb) bt
>>     #0  0x00007f3bb77a0533 in virDomainDefHasVcpusOffline (def=def at 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 at entry=0x7f3b7802a830, caps=0x7f3b70000ae0, flags=3, flags at entry=1, 
>>         buf=buf at entry=0x7f3ba7308940, xmlopt=xmlopt at entry=0x0) at conf/domain_conf.c:27850
>>     #3  0x00007f3bb77ced46 in virDomainDefFormat (def=def at entry=0x7f3b7802a830, caps=<optimized out>, 
>>         flags=flags at 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 at 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