Re: [libvirt] [PATCH 1/3] libvirt: Increase connection reference count for callbacks
On Wed, Mar 27, 2013 at 11:16:26AM +0100, Peter Krempa wrote:
On 03/26/13 10:54, Viktor Mihajlovski wrote:
>By adjusting the reference count of the connection object we
>prevent races between callback function and virConnectClose.
Hum. Here I agree that this is definitely possible (and I managed to
reproduce the invalid read and possible memory corruption) but I
don't completely like the fix here.
As the callback is called at a moment when the connection won't be
usable anymore I think that the callback should automatically
unregister itself and also clear the opaque data if that is
requested. (Which isn't done right now if the caller doesn't
unregister it).
This only works if we can guarantee that the callback is invoked in
100% of paths that lead to the connection closing. This isn't the
case for a client initiated close operation. So we're still left with
a need to manually unregister the callback, except now instead of
having todo that every time, we only need todo it some of the time.
Either we require apps to never unregister it, or we require apps
to always unregister - anything inbetweeen is just bad.
With automatic unregistration we save a lot of hassle, and also
avoid the need of hackery that you needed to add in the 3/3 patch to
avoid leaking the reference. I also think that the virConnectClose
function should automatically get rid of the callback if the caller
doesn't do it before.
The virConnectClose function is just a thin wrapper around virObjectUnref.
To explicitly remove the callback here would require that you look at the
reference count, but doing so is inherantly racey, which is why the
virObjectUnref method only returns a boolean, indicating whether the object
has been freed, instead of the actual ref count.
Requiring apps to unregister the callbacks is the same that we have
with existing domain lifecycle callbacks, so is not unexpected / out
of the ordinary for applications.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|