[libvirt] [PATCH 2/2] tests: qemuhotplug: Don't free the monitor object as part of @vm
Peter Krempa
pkrempa at redhat.com
Thu Feb 2 16:35:35 UTC 2017
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 at 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 at 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 at linux.vnet.ibm.com>
Thanks, I'll push this in a while.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170202/1945be7b/attachment-0001.sig>
More information about the libvir-list
mailing list