[libvirt] [PATCH 1/3] libvirt: Increase connection reference count for callbacks

[Peter Krempa]

On 03/27/13 11:51, Daniel P. Berrange wrote:
> 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.


Okay, in that case I can't see a better solution for now other than 
taking this patch.


>
> Daniel
>

Peter