[libvirt] [PATCH 0/2] Fix random crash in qemuhotplugtest
First patch makes the bug 100%[1] reproducible. *[1]: Race conditions may apply. Peter Krempa (2): DON'T APPLY: Reproducer to serialize threads "correctly" tests: qemuhotplug: Don't free the monitor object as part of @vm tests/qemuhotplugtest.c | 10 +++++++++- tests/qemumonitortestutils.c | 2 ++ 2 files changed, 11 insertions(+), 1 deletion(-) -- 2.11.0
--- tests/qemumonitortestutils.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index cfd0a38cb..98ea1266c 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -435,6 +435,8 @@ qemuMonitorTestFree(qemuMonitorTestPtr test) qemuAgentClose(test->agent); } + sleep(1); + virObjectUnref(test->vm); if (test->started) -- 2.11.0
The test monitor should be freed separately so we need to remove the pointer from the @vm object. This fixes a race condition crash in the test introduced in commit a245abce43. --- tests/qemuhotplugtest.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 8cceb883e..8a58d5468 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -365,6 +365,8 @@ struct testQemuHotplugCpuData { static void testQemuHotplugCpuDataFree(struct testQemuHotplugCpuData *data) { + qemuDomainObjPrivatePtr priv; + if (!data) return; @@ -375,7 +377,13 @@ testQemuHotplugCpuDataFree(struct testQemuHotplugCpuData *data) VIR_FREE(data->xml_dom); - virObjectUnref(data->vm); + if (data->vm) { + priv = data->vm->privateData; + priv->mon = NULL; + + virObjectUnref(data->vm); + } + qemuMonitorTestFree(data->mon); VIR_FREE(data); } -- 2.11.0
On Thu, Feb 02, 2017 at 04:46 PM +0100, Peter Krempa <pkrempa@redhat.com> wrote:
The test monitor should be freed separately so we need to remove the pointer from the @vm object. This fixes a race condition crash in the test introduced in commit a245abce43. --- tests/qemuhotplugtest.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 8cceb883e..8a58d5468 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -365,6 +365,8 @@ struct testQemuHotplugCpuData { static void testQemuHotplugCpuDataFree(struct testQemuHotplugCpuData *data) { + qemuDomainObjPrivatePtr priv; + if (!data) return;
@@ -375,7 +377,13 @@ testQemuHotplugCpuDataFree(struct testQemuHotplugCpuData *data)
VIR_FREE(data->xml_dom);
- virObjectUnref(data->vm); + if (data->vm) { + priv = data->vm->privateData; + priv->mon = NULL; + + virObjectUnref(data->vm); + } + qemuMonitorTestFree(data->mon); VIR_FREE(data); } -- 2.11.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Just a question to this. Currently we get the reference to the monitor with 'priv->mon = qemuMonitorTestGetMonitor(data->mon);' (testQemuHotplugCpuPrepare in tests/qemuhotplugtest.c). Shouldn't we use 'priv->mon = virObjectRef(qemuMonitorTestGetMonitor(data->mon));' for getting the reference (and later on 'virObjectUnref(priv->mon); priv->mon = NULL;')? I know your fix works but I'm still thinking about that. Anyway, Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Thu, Feb 02, 2017 at 17:12:28 +0100, Marc Hartmayer wrote:
On Thu, Feb 02, 2017 at 04:46 PM +0100, Peter Krempa <pkrempa@redhat.com> wrote:
The test monitor should be freed separately so we need to remove the pointer from the @vm object. This fixes a race condition crash in the test introduced in commit a245abce43. --- tests/qemuhotplugtest.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 8cceb883e..8a58d5468 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -365,6 +365,8 @@ struct testQemuHotplugCpuData { static void testQemuHotplugCpuDataFree(struct testQemuHotplugCpuData *data) { + qemuDomainObjPrivatePtr priv; + if (!data) return;
@@ -375,7 +377,13 @@ testQemuHotplugCpuDataFree(struct testQemuHotplugCpuData *data)
VIR_FREE(data->xml_dom);
- virObjectUnref(data->vm); + if (data->vm) { + priv = data->vm->privateData; + priv->mon = NULL; + + virObjectUnref(data->vm); + } + qemuMonitorTestFree(data->mon); VIR_FREE(data); } -- 2.11.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Just a question to this.
Currently we get the reference to the monitor with 'priv->mon = qemuMonitorTestGetMonitor(data->mon);' (testQemuHotplugCpuPrepare in tests/qemuhotplugtest.c).
Shouldn't we use 'priv->mon = virObjectRef(qemuMonitorTestGetMonitor(data->mon));' for getting the reference (and later on 'virObjectUnref(priv->mon); priv->mon = NULL;')?
I know your fix works but I'm still thinking about that.
Well, since we are only borrowing the reference to the vm object I don't think it would be necessary to do the ref-dance. In that case we should also do it for the second function doing similar things. I also thoguht that we could ref it and then let the destructor for @vm to destroy it. I'm not sure whether that wouldn't do a cyclic reference though so I sticked with the simplest solution ... at least for the tests.
Anyway, Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Thanks, I'll push this in a while.
participants (2)
-
Marc Hartmayer -
Peter Krempa