[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]