On 06.12.2016 15:39, Cédric Bosdonnat wrote: > If the monitor doesn't hold a reference to the domain object > the object may be destroyed before the monitor actually stops. > --- > src/lxc/lxc_monitor.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/lxc/lxc_monitor.c b/src/lxc/lxc_monitor.c > index d828d52..de63f9e 100644 > --- a/src/lxc/lxc_monitor.c > +++ b/src/lxc/lxc_monitor.c > @@ -179,6 +179,7 @@ virLXCMonitorPtr virLXCMonitorNew(virDomainObjPtr vm, > memcpy(&mon->cb, cb, sizeof(mon->cb)); > > virObjectRef(mon); > + virObjectRef(vm); You can move this a few lines above: mon->vm = virObjectRef(vm); if you want. > virNetClientSetCloseCallback(mon->client, virLXCMonitorEOFNotify, mon, > virLXCMonitorCloseFreeCallback); > > @@ -188,6 +189,7 @@ virLXCMonitorPtr virLXCMonitorNew(virDomainObjPtr vm, > > error: > virObjectUnref(mon); > + virObjectUnref(vm); This doesn't feel right. Imagine something bad happened before @vm got ref'd (the first hunk). The control jumps over to error label and unref @vm even though it hasn't been ref'd yet. Or worse - @mon has exactly one reference (we are creating it here in this function), therefore the above unref(mon) causes the MonitorDispose callback to be called, which unrefs the @vm again. Fortunately, this scenario is currently impossible as there's no 'goto error' after @mon->vm is set, but you get my point. > mon = NULL; > goto cleanup; > } > @@ -201,6 +203,7 @@ static void virLXCMonitorDispose(void *opaque) > if (mon->cb.destroy) > (mon->cb.destroy)(mon, mon->vm); > virObjectUnref(mon->program); > + virObjectUnref(mon->vm); > } > > > ACK if you drop the 2nd hunk. Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
