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