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

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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list