9:46 a.m.
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
9:46 a.m.
New subject: [libvirt] [PATCH 1/2] DON'T APPLY: Reproducer to serialize threads "correctly"
---
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
9:46 a.m.
New subject: [libvirt] [PATCH 2/2] tests: qemuhotplug: Don't free the monitor object as part of @vm
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
10:12 a.m.
New subject: [libvirt] [PATCH 2/2] tests: qemuhotplug: Don't free the monitor object as part of @vm
On Thu, Feb 02, 2017 at 04:46 PM +0100, Peter Krempa <pkrempa(a)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(a)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(a)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
10:35 a.m.
New subject: [libvirt] [PATCH 2/2] tests: qemuhotplug: Don't free the monitor object as part of @vm
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(a)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(a)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(a)linux.vnet.ibm.com>
Thanks, I'll push this in a while.
2854
days inactive
2854
days old
4 comments
2 participants
participants (2)
-
Marc Hartmayer -
Peter Krempa