Re: [libvirt] [PATCH] remote: Prevent race when closing a connection
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