[asterisk-commits] rmudgett: branch rmudgett/masq_asterisk_19537 r364257 - /team/rmudgett/masq_a...

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Fri Apr 27 13:09:53 CDT 2012


Author: rmudgett
Date: Fri Apr 27 13:09:48 2012
New Revision: 364257

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=364257
Log:
Rearange ast_do_masquerade.  It can no longer fail so no need to check return value.

Modified:
    team/rmudgett/masq_asterisk_19537/main/channel.c

Modified: team/rmudgett/masq_asterisk_19537/main/channel.c
URL: http://svnview.digium.com/svn/asterisk/team/rmudgett/masq_asterisk_19537/main/channel.c?view=diff&rev=364257&r1=364256&r2=364257
==============================================================================
--- team/rmudgett/masq_asterisk_19537/main/channel.c (original)
+++ team/rmudgett/masq_asterisk_19537/main/channel.c Fri Apr 27 13:09:48 2012
@@ -2762,13 +2762,7 @@
 	 */
 	while (chan->masq) {
 		ast_channel_unlock(chan);
-		if (ast_do_masquerade(chan)) {
-			ast_log(LOG_WARNING, "Failed to perform masquerade\n");
-
-			/* Abort the loop or we might never leave. */
-			ast_channel_lock(chan);
-			break;
-		}
+		ast_do_masquerade(chan);
 		ast_channel_lock(chan);
 	}
 
@@ -2784,6 +2778,7 @@
 		return 0;
 	}
 
+	/* Mark as a zombie so a masquerade cannot be setup on this channel. */
 	if (!(was_zombie = ast_test_flag(chan, AST_FLAG_ZOMBIE))) {
 		ast_set_flag(chan, AST_FLAG_ZOMBIE);
 	}
@@ -3144,10 +3139,8 @@
 	
 	/* Perform any pending masquerades */
 	for (x = 0; x < n; x++) {
-		if (c[x]->masq && ast_do_masquerade(c[x])) {
-			ast_log(LOG_WARNING, "Masquerade failed\n");
-			*ms = -1;
-			return NULL;
+		while (c[x]->masq) {
+			ast_do_masquerade(c[x]);
 		}
 
 		ast_channel_lock(c[x]);
@@ -3278,10 +3271,8 @@
 
 
 	/* See if this channel needs to be masqueraded */
-	if (chan->masq && ast_do_masquerade(chan)) {
-		ast_log(LOG_WARNING, "Failed to perform masquerade on %s\n", chan->name);
-		*ms = -1;
-		return NULL;
+	while (chan->masq) {
+		ast_do_masquerade(chan);
 	}
 
 	ast_channel_lock(chan);
@@ -3360,10 +3351,8 @@
 	struct ast_channel *winner = NULL;
 
 	for (i = 0; i < n; i++) {
-		if (c[i]->masq && ast_do_masquerade(c[i])) {
-			ast_log(LOG_WARNING, "Masquerade failed\n");
-			*ms = -1;
-			return NULL;
+		while (c[i]->masq) {
+			ast_do_masquerade(c[i]);
 		}
 
 		ast_channel_lock(c[i]);
@@ -3742,11 +3731,8 @@
 	 */
 
 	if (chan->masq) {
-		if (ast_do_masquerade(chan))
-			ast_log(LOG_WARNING, "Failed to perform masquerade\n");
-		else
-			f =  &ast_null_frame;
-		return f;
+		ast_do_masquerade(chan);
+		return &ast_null_frame;
 	}
 
 	/* if here, no masq has happened, lock the channel and proceed */
@@ -4804,12 +4790,9 @@
 		goto done;
 
 	/* Handle any pending masquerades */
-	if (chan->masq) {
+	while (chan->masq) {
 		ast_channel_unlock(chan);
-		if (ast_do_masquerade(chan)) {
-			ast_log(LOG_WARNING, "Failed to perform masquerade\n");
-			return res; /* no need to goto done: chan is already unlocked for masq */
-		}
+		ast_do_masquerade(chan);
 		ast_channel_lock(chan);
 	}
 	if (chan->masqr) {
@@ -6102,11 +6085,13 @@
 void ast_change_name(struct ast_channel *chan, const char *newname)
 {
 	/* We must re-link, as the hash value will change here. */
+	ao2_lock(channels);
+	ast_channel_lock(chan);
 	ao2_unlink(channels, chan);
-	ast_channel_lock(chan);
 	__ast_change_name_nolink(chan, newname);
+	ao2_link(channels, chan);
 	ast_channel_unlock(chan);
-	ao2_link(channels, chan);
+	ao2_unlock(channels);
 }
 
 void ast_channel_inherit_variables(const struct ast_channel *parent, struct ast_channel *child)
@@ -6163,8 +6148,7 @@
 	struct ast_var_t *current, *newvar;
 	/* Append variables from clone channel into original channel */
 	/* XXX Is this always correct?  We have to in order to keep MACROS working XXX */
-	if (AST_LIST_FIRST(&clonechan->varshead))
-		AST_LIST_APPEND_LIST(&original->varshead, &clonechan->varshead, entries);
+	AST_LIST_APPEND_LIST(&original->varshead, &clonechan->varshead, entries);
 
 	/* then, dup the varshead list into the clone */
 	
@@ -6414,11 +6398,11 @@
  */
 int ast_do_masquerade(struct ast_channel *original)
 {
-	format_t x;
+	int x;
 	int i;
-	int res=0;
 	int origstate;
 	int visible_indication;
+	int clone_was_zombie = 0;/*!< TRUE if the clonechan was a zombie before the masquerade. */
 	struct ast_frame *current;
 	const struct ast_channel_tech *t;
 	void *t_pvt;
@@ -6433,8 +6417,9 @@
 	struct ast_cdr *cdr;
 	struct ast_datastore *xfer_ds;
 	struct xfer_masquerade_ds *xfer_colp;
-	format_t rformat = original->readformat;
-	format_t wformat = original->writeformat;
+	format_t rformat;
+	format_t wformat;
+	format_t tmp_format;
 	char newn[AST_CHANNEL_NAME];
 	char orig[AST_CHANNEL_NAME];
 	char masqn[AST_CHANNEL_NAME];
@@ -6445,52 +6430,59 @@
 	 * 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.
+	/*
+	 * The reasoning for the channels ao2_container lock here is
+	 * complex.
+	 *
+	 * There is a race condition that exists for this function.
+	 * Since all pvt and channel locks must be let go before calling
+	 * ast_do_masquerade, it is possible that it could be called
+	 * multiple times for the same channel.  In order to prevent the
+	 * race condition with competing threads to do the masquerade
+	 * and new masquerade attempts, the channels container must be
+	 * locked for the entire masquerade.  The original and clonechan
+	 * need to be unlocked earlier to avoid potential deadlocks with
+	 * the chan_local deadlock avoidance method.
+	 *
+	 * The container lock blocks competing masquerade attempts from
+	 * starting as well as being necessary for proper locking order
+	 * because the channels must to be unlinked to change their
+	 * names.
+	 *
+	 * The original and clonechan locks must be held while the
+	 * channel contents are shuffled around for the masquerade.
+	 *
+	 * The masq and masqr pointers need to be left alone until the
+	 * masquerade has restabilized the channels to prevent another
+	 * masquerade request until the AST_FLAG_ZOMBIE can be set on
+	 * the clonechan.
 	 */
 	ao2_lock(channels);
 
-	/* lock the original channel to determine if the masquerade is required or not */
+	/*
+	 * Lock the original channel to determine if the masquerade is
+	 * still required.
+	 */
 	ast_channel_lock(original);
 
-	/*
-	 * This checks to see if the masquerade has already happened or
-	 * not.  There is a race condition that exists for this
-	 * function.  Since all pvt and channel locks must be let go
-	 * before calling do_masquerade, it is possible that it could be
-	 * called multiple times for the same channel.  This check
-	 * verifies whether or not the masquerade has already been
-	 * completed by another thread.
-	 */
-	while ((clonechan = original->masq) && ast_channel_trylock(clonechan)) {
+	clonechan = original->masq;
+	if (!clonechan) {
 		/*
-		 * A masq is needed but we could not get the clonechan lock
-		 * immediately.  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.
+		 * The masq is already completed by another thread or never
+		 * needed to be done to begin with.
 		 */
-		CHANNEL_DEADLOCK_AVOIDANCE(original);
-	}
-
-	/*
-	 * A final masq check must be done after deadlock avoidance for
-	 * clonechan above or we could get a double masq.  This is
-	 * posible with ast_hangup at least.
-	 */
-	if (!clonechan) {
-		/* masq already completed by another thread, or never needed to be done to begin with */
 		ast_channel_unlock(original);
 		ao2_unlock(channels);
 		return 0;
 	}
+
+	/* Bump the refs to ensure that they won't dissapear on us. */
+	ast_channel_ref(original);
+	ast_channel_ref(clonechan);
+
+	/* unlink from channels container as name (which is the hash value) will change */
+	ao2_unlink(channels, original);
+	ao2_unlink(channels, clonechan);
 
 	/* Get any transfer masquerade connected line exchange data. */
 	xfer_ds = ast_channel_datastore_find(original, &xfer_ds_info, NULL);
@@ -6502,30 +6494,26 @@
 	}
 
 	/*
-	 * Release any hold on the transferee channel before proceeding
-	 * with the masquerade.
-	 */
-	if (xfer_colp && xfer_colp->transferee_held) {
-		ast_indicate(clonechan, AST_CONTROL_UNHOLD);
-	}
-
-	/* clear the masquerade channels */
-	original->masq = NULL;
-	clonechan->masqr = NULL;
-
-	/* unlink from channels container as name (which is the hash value) will change */
-	ao2_unlink(channels, original);
-	ao2_unlink(channels, clonechan);
-
-	ast_debug(4, "Actually Masquerading %s(%d) into the structure of %s(%d)\n",
-		clonechan->name, clonechan->_state, original->name, original->_state);
-
-	/*
 	 * Stop any visible indiction on the original channel so we can
 	 * transfer it to the clonechan taking the original's place.
 	 */
 	visible_indication = original->visible_indication;
+	ast_channel_unlock(original);
 	ast_indicate(original, -1);
+
+	/*
+	 * Release any hold on the transferee channel before going any
+	 * further with the masquerade.
+	 */
+	if (xfer_colp && xfer_colp->transferee_held) {
+		ast_indicate(clonechan, AST_CONTROL_UNHOLD);
+	}
+
+	/* Start the masquerade channel contents rearangement. */
+	ast_channel_lock_both(original, clonechan);
+
+	ast_debug(4, "Actually Masquerading %s(%d) into the structure of %s(%d)\n",
+		clonechan->name, clonechan->_state, original->name, original->_state);
 
 	chans[0] = clonechan;
 	chans[1] = original;
@@ -6536,8 +6524,12 @@
 		"OriginalState: %s\r\n",
 		clonechan->name, ast_state2str(clonechan->_state), original->name, ast_state2str(original->_state));
 
-	/* Having remembered the original read/write formats, we turn off any translation on either
-	   one */
+	/*
+	 * Remember the original read/write formats.  We turn off any
+	 * translation on either one
+	 */
+	rformat = original->readformat;
+	wformat = original->writeformat;
 	free_translation(clonechan);
 	free_translation(original);
 
@@ -6562,14 +6554,14 @@
 	original->tech = clonechan->tech;
 	clonechan->tech = t;
 
+	t_pvt = original->tech_pvt;
+	original->tech_pvt = clonechan->tech_pvt;
+	clonechan->tech_pvt = t_pvt;
+
 	/* Swap the cdrs */
 	cdr = original->cdr;
 	original->cdr = clonechan->cdr;
 	clonechan->cdr = cdr;
-
-	t_pvt = original->tech_pvt;
-	original->tech_pvt = clonechan->tech_pvt;
-	clonechan->tech_pvt = t_pvt;
 
 	/* Swap the alertpipes */
 	for (i = 0; i < 2; i++) {
@@ -6578,7 +6570,7 @@
 		clonechan->alertpipe[i] = x;
 	}
 
-	/* 
+	/*
 	 * Swap the readq's.  The end result should be this:
 	 *
 	 *  1) All frames should be on the new (original) channel.
@@ -6591,8 +6583,8 @@
 	 */
 	{
 		AST_LIST_HEAD_NOLOCK(, ast_frame) tmp_readq;
-		AST_LIST_HEAD_SET_NOLOCK(&tmp_readq, NULL);
-
+
+		AST_LIST_HEAD_INIT_NOLOCK(&tmp_readq);
 		AST_LIST_APPEND_LIST(&tmp_readq, &original->readq, frame_list);
 		AST_LIST_APPEND_LIST(&original->readq, &clonechan->readq, frame_list);
 
@@ -6609,12 +6601,13 @@
 	}
 
 	/* Swap the raw formats */
-	x = original->rawreadformat;
+	tmp_format = original->rawreadformat;
 	original->rawreadformat = clonechan->rawreadformat;
-	clonechan->rawreadformat = x;
-	x = original->rawwriteformat;
+	clonechan->rawreadformat = tmp_format;
+
+	tmp_format = original->rawwriteformat;
 	original->rawwriteformat = clonechan->rawwriteformat;
-	clonechan->rawwriteformat = x;
+	clonechan->rawwriteformat = tmp_format;
 
 	clonechan->_softhangup = AST_SOFTHANGUP_DEV;
 
@@ -6625,23 +6618,6 @@
 	origstate = original->_state;
 	original->_state = clonechan->_state;
 	clonechan->_state = origstate;
-
-	if (clonechan->tech->fixup && clonechan->tech->fixup(original, clonechan)) {
-		ast_log(LOG_WARNING, "Fixup failed on channel %s, strange things may happen.\n", clonechan->name);
-	}
-
-	/* Start by disconnecting the original's physical side */
-	if (clonechan->tech->hangup && clonechan->tech->hangup(clonechan)) {
-		ast_log(LOG_WARNING, "Hangup failed!  Strange things may happen!\n");
-		res = -1;
-		goto done;
-	}
-
-	/*
-	 * We just hung up the physical side of the channel.  Set the
-	 * new zombie to use the kill channel driver for safety.
-	 */
-	clonechan->tech = &ast_kill_tech;
 
 	/* Mangle the name of the clone channel */
 	snprintf(zombn, sizeof(zombn), "%s<ZOMBIE>", orig); /* quick, hide the brains! */
@@ -6743,37 +6719,108 @@
 	ast_debug(1, "Putting channel %s in %s/%s formats\n", original->name,
 		ast_getformatname(wformat), ast_getformatname(rformat));
 
-	/* Okay.  Last thing is to let the channel driver know about all this mess, so he
-	   can fix up everything as best as possible */
-	if (original->tech->fixup) {
-		if (original->tech->fixup(clonechan, original)) {
-			ast_log(LOG_WARNING, "Channel for type '%s' could not fixup channel %s\n",
-				original->tech->type, original->name);
-			res = -1;
-			goto done;
-		}
-	} else
-		ast_log(LOG_WARNING, "Channel type '%s' does not have a fixup routine (for %s)!  Bad things may happen.\n",
+	/* Fixup the original clonechan's physical side */
+	if (original->tech->fixup && original->tech->fixup(clonechan, original)) {
+		ast_log(LOG_WARNING, "Channel type '%s' could not fixup channel %s, strange things may happen. (clonechan)\n",
 			original->tech->type, original->name);
-
-	/* 
-	 * If an indication is currently playing, maintain it on the channel 
-	 * that is taking the place of original 
+	}
+
+	/* Fixup the original original's physical side */
+	if (clonechan->tech->fixup && clonechan->tech->fixup(original, clonechan)) {
+		ast_log(LOG_WARNING, "Channel type '%s' could not fixup channel %s, strange things may happen. (original)\n",
+			clonechan->tech->type, clonechan->name);
+	}
+
+	/*
+	 * Now, at this point, the "clone" channel is totally F'd up.
+	 * We mark it as a zombie so nothing tries to touch it.  If it's
+	 * already been marked as a zombie, then we must free it (since
+	 * it already is considered invalid).
 	 *
-	 * This is needed because the masquerade is swapping out in the internals
-	 * of this channel, and the new channel private data needs to be made
-	 * aware of the current visible indication (RINGING, CONGESTION, etc.)
+	 * This must be done before we unlock clonechan to prevent
+	 * setting up another masquerade on the clonechan.
+	 */
+	if (ast_test_flag(clonechan, AST_FLAG_ZOMBIE)) {
+		clone_was_zombie = 1;
+	} else {
+		ast_set_flag(clonechan, AST_FLAG_ZOMBIE);
+		ast_queue_frame(clonechan, &ast_null_frame);
+	}
+
+	/* clear the masquerade channels */
+	original->masq = NULL;
+	clonechan->masqr = NULL;
+
+	/*
+	 * When we unlock original here, it can be immediately setup to
+	 * masquerade again or hungup.  The new masquerade or hangup
+	 * will not actually happen until we release the channels
+	 * container lock.
+	 */
+	ast_channel_unlock(original);
+
+	/* Disconnect the original original's physical side */
+	if (clonechan->tech->hangup && clonechan->tech->hangup(clonechan)) {
+		ast_log(LOG_WARNING, "Hangup failed!  Strange things may happen!\n");
+	} else {
+		/*
+		 * We just hung up the original original's physical side of the
+		 * channel.  Set the new zombie to use the kill channel driver
+		 * for safety.
+		 */
+		clonechan->tech = &ast_kill_tech;
+	}
+
+	ast_channel_unlock(clonechan);
+
+	/*
+	 * If an indication is currently playing, maintain it on the
+	 * channel that is taking the place of original.
+	 *
+	 * This is needed because the masquerade is swapping out the
+	 * internals of the channel, and the new channel private data
+	 * needs to be made aware of the current visible indication
+	 * (RINGING, CONGESTION, etc.)
 	 */
 	if (visible_indication) {
 		ast_indicate(original, visible_indication);
 	}
 
-	/* Now, at this point, the "clone" channel is totally F'd up.  We mark it as
-	   a zombie so nothing tries to touch it.  If it's already been marked as a
-	   zombie, then free it now (since it already is considered invalid). */
-	if (ast_test_flag(clonechan, AST_FLAG_ZOMBIE)) {
+	ast_channel_lock(original);
+
+	/* Signal any blocker */
+	if (ast_test_flag(original, AST_FLAG_BLOCKING)) {
+		pthread_kill(original->blocker, SIGURG);
+	}
+
+	ast_debug(1, "Done Masquerading %s (%d)\n", original->name, original->_state);
+
+	if ((bridged = ast_bridged_channel(original))) {
+		ast_channel_ref(bridged);
+		ast_channel_unlock(original);
+		ast_indicate(bridged, AST_CONTROL_SRCCHANGE);
+		ast_channel_unref(bridged);
+	} else {
+		ast_channel_unlock(original);
+	}
+	ast_indicate(original, AST_CONTROL_SRCCHANGE);
+
+	if (xfer_colp) {
+		/*
+		 * After the masquerade, the original channel pointer actually
+		 * points to the new transferee channel and the bridged channel
+		 * is still the intended transfer target party.
+		 */
+		masquerade_colp_transfer(original, xfer_colp);
+	}
+
+	if (xfer_ds) {
+		ast_datastore_free(xfer_ds);
+	}
+
+	if (clone_was_zombie) {
+		ast_channel_lock(clonechan);
 		ast_debug(1, "Destroying channel clone '%s'\n", clonechan->name);
-		ast_channel_unlock(clonechan);
 		ast_manager_event(clonechan, EVENT_FLAG_CALL, "Hangup",
 			"Channel: %s\r\n"
 			"Uniqueid: %s\r\n"
@@ -6784,52 +6831,25 @@
 			clonechan->hangupcause,
 			ast_cause2str(clonechan->hangupcause)
 			);
-		clonechan = ast_channel_release(clonechan);
+		ast_channel_unlock(clonechan);
+
+		/*
+		 * Drop the system reference to destroy the channel since it is
+		 * already unlinked.
+		 */
+		ast_channel_unref(clonechan);
 	} else {
-		ast_debug(1, "Released clone lock on '%s'\n", clonechan->name);
-		ast_set_flag(clonechan, AST_FLAG_ZOMBIE);
-		ast_queue_frame(clonechan, &ast_null_frame);
-	}
-
-	/* Signal any blocker */
-	if (ast_test_flag(original, AST_FLAG_BLOCKING))
-		pthread_kill(original->blocker, SIGURG);
-	ast_debug(1, "Done Masquerading %s (%d)\n", original->name, original->_state);
-
-	if ((bridged = ast_bridged_channel(original))) {
-		ast_channel_lock(bridged);
-		ast_indicate(bridged, AST_CONTROL_SRCCHANGE);
-		ast_channel_unlock(bridged);
-	}
-	ast_indicate(original, AST_CONTROL_SRCCHANGE);
-
-	if (xfer_colp) {
-		/*
-		 * After the masquerade, the original channel pointer actually
-		 * points to the new transferee channel and the bridged channel
-		 * is still the intended transfer target party.
-		 */
-		masquerade_colp_transfer(original, xfer_colp);
-	}
-
-done:
-	if (xfer_ds) {
-		ast_datastore_free(xfer_ds);
-	}
-	/* 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);
-		ao2_link(channels, original);
-	} else {
-		ast_channel_unlock(original);
-		ao2_link(channels, original);
-	}
-
+	}
+
+	ao2_link(channels, original);
 	ao2_unlock(channels);
 
-	return res;
+	/* Release our held safety references. */
+	ast_channel_unref(original);
+	ast_channel_unref(clonechan);
+
+	return 0;
 }
 
 void ast_set_callerid(struct ast_channel *chan, const char *cid_num, const char *cid_name, const char *cid_ani)




More information about the asterisk-commits mailing list