[svn-commits] dvossel: branch dvossel/masq_locking_order_trunk r222151 - /team/dvossel/masq...

[SVN commits to the Digium repositories]

Author: dvossel
Date: Mon Oct  5 18:08:36 2009
New Revision: 222151

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=222151
Log:
In ast_do_masquerade, the channels ao2_container does not have
to be held the entire function.  These changes make it safe
to let the container lock go after unlinking takes place.


Modified:
    team/dvossel/masq_locking_order_trunk/main/channel.c

Modified: team/dvossel/masq_locking_order_trunk/main/channel.c
URL: http://svnview.digium.com/svn/asterisk/team/dvossel/masq_locking_order_trunk/main/channel.c?view=diff&rev=222151&r1=222150&r2=222151
==============================================================================
--- team/dvossel/masq_locking_order_trunk/main/channel.c (original)
+++ team/dvossel/masq_locking_order_trunk/main/channel.c Mon Oct  5 18:08:36 2009
@@ -5109,8 +5109,18 @@
 	 * original channel's backend.  While the features are nice, which is the
 	 * reason we're keeping it, it's still awesomely weird. XXX */
 
+	/* The reasoning for the channels ao2_container lock here is complex.
+	 * 
+	 * In order to check for a race condition, the original channel must
+	 * be locked.  If it is determined that the masquerade should proceed
+	 * the original channel can absolutely not be unlocked until the end
+	 * of the function.  Since after determining the masquerade should
+	 * continue requires the channels to be unlinked from the ao2_container,
+	 * the container lock must be held first to achieve proper locking order.
+	 */
 	ao2_lock(channels);
 
+	/* lock the original channel to determine if the masquerade is require or not */
 	ast_channel_lock(original);
 
 	/* This checks to see if the masquerade has already happened or not.  There is a
@@ -5127,8 +5137,10 @@
 	/* now that we have verified no race condition exists, set the clone channel */
 	clonechan = original->masq;
 
-	/* since this function already holds the global containers lock, unlocking original
-	 * for deadlock avoidance will not result in a sort of masquerade race condition */
+	/* since this function already holds the global container lock, unlocking original
+	 * for deadlock avoidance will not result in any sort of masquerade race condition.
+	 * If masq is called by a different thread while this happens, it will be stuck waiting
+	 * until we unlock the container. */
 	while (ast_channel_trylock(clonechan)) {
 		CHANNEL_DEADLOCK_AVOIDANCE(original);
 	}
@@ -5140,6 +5152,9 @@
 	/* unlink from channels container as name (which is the hash value) will change */
 	ao2_unlink(channels, original);
 	ao2_unlink(channels, clonechan);
+
+	/* now that both channels are locked and unlinked from the container, it is safe to unlock it */
+	ao2_unlock(channels);
 
 	ast_debug(4, "Actually Masquerading %s(%d) into the structure of %s(%d)\n",
 		clonechan->name, clonechan->_state, original->name, original->_state);
@@ -5397,12 +5412,14 @@
 done:
 	/* it is possible for the clone channel to disappear during this */
 	if (clonechan) {
+		ast_channel_unlock(original);
 		ast_channel_unlock(clonechan);
 		ao2_link(channels, clonechan);
-	}
-	ast_channel_unlock(original);
-	ao2_link(channels, original);
-	ao2_unlock(channels);
+		ao2_link(channels, original);
+	} else {
+		ast_channel_unlock(original);
+		ao2_link(channels, original);
+	}
 	return 0;
 }