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