[libvirt] [PATCH] qemu: aquire job in qemuDomainDefineXMLFlags
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@virtuozzo.com> --- 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; + if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, 0, &oldDef))) @@ -7685,6 +7690,9 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); cleanup: + if (job) + qemuDomainObjEndJob(driver, vm); + virDomainDefFree(oldDef); virDomainDefFree(def); virDomainObjEndAPI(&vm); -- 1.8.3.1
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@virtuozzo.com> ---
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)))
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@virtuozzo.com> ---
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
On 5/28/19 1:34 PM, 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@virtuozzo.com> ---
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(+)
I've managed to reproduce this. It's really simple. I've put a sleep() here (to give myself more time to run commands): diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 42b1ce2521..a5fe71e189 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -2405,6 +2405,7 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period, qemuDomainObjEnterMonitor(driver, vm); r = qemuMonitorSetMemoryStatsPeriod(priv->mon, def->memballoon, period); + sleep(60); if (qemuDomainObjExitMonitor(driver, vm) < 0) goto endjob; if (r < 0) { And then: 1) virsh start $domain 2) virsh dommemstat --period 10 --config --live $domain 3) [from a different terminal] virsh edit $domain # just add an empty line somewhere to enforce redefining the domain 4) Observe crashed daemon
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; + if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, 0, &oldDef)))
This won't work really. @vm is NULL at the point of BeginJob() and the first thing that BeginJob() does is it dereferences @vm. At the same time, we can't just blindly put BeginJob() into virDomainObjListAdd() because that one is driver agnostic and BeginJob() is a qemu function. Maybe we can extend xmlopt for a new callback that if set is called just before a domain definition is changed. Then qemu driver would set this callback to BeginJob(). But this creates a problem because acquiring a job can take up to QEMU_JOB_WAIT_TIME seconds (currently 30) during which domain list is locked for write, i.e. not even 'virsh list' would be able to run meanwhile. Michal
participants (3)
-
Michal Privoznik -
Nikolay Shirokovskiy -
Peter Krempa