[libvirt] [PATCH] remote: Prevent race when closing a connection

[Viktor Mihajlovski]

On 03/15/2013 12:28 AM, Eric Blake wrote:
>> -    if (conn->closeFreeCallback)
>> +    if (conn->closeCallback)
>> +        conn->closeCallback = NULL;
>
> The if is pointless.  Just blindly set conn->closeCallback to NULL.
>
agreed
>> +
>> +    if (conn->closeFreeCallback) {
>>           conn->closeFreeCallback(conn->closeOpaque);
>> +        conn->closeFreeCallback = NULL;
>> +        conn->closeOpaque = NULL;
>
> Clearing conn->closeOpaque is pointless; it is only ever used depending
> on conn->closeFreeCallback, and leaving it non-NULL doesn't hurt.
>
I know, and didn't do it initially, but then wanted to make it common
with the callback deregistering code. And a small portion of paranoia
doesn't hurt as I have come to learn.
>
> ...Wouldn't it be better to stash a copy of the callback pointer while
> the mutex is held, but avoid calling the callback until after the mutex
> is unlocked?  Something like:
>
> <TYPE> cb = NULL;
> void* opaque;
> virMutexLock(&conn->lock);
> conn->closeDispatch = false;
> if (conn->closeUnregisterCount != closeUnregisterCount) {
>      cb = closeFreeCallback;
>      opaque = closeOpaque;
> }
> virMutexUnlock(&conn->lock);
> if (cb)
>      cb(opaque);
>
maybe, but this is again common to the other places where the
freeing callback is invoked, i.e. within the lock.

Waiting for Dan's comments...

-- 

Mit freundlichen Grüßen/Kind Regards
    Viktor Mihajlovski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294