[asterisk-commits] mmichelson: branch mmichelson/atxfer_features r392917 - /team/mmichelson/atxf...

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Tue Jun 25 18:05:26 CDT 2013


Author: mmichelson
Date: Tue Jun 25 18:05:24 2013
New Revision: 392917

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=392917
Log:
Fix up some referencing issues.

There was a refleak in my move and merge functions since bridge channels
were not being unreffed.

There was also a misunderstanding about ast_bridge_impart's practice of
gobbling the reference of the channel that is being added. This led to
an overabundance of unrefs for the transfer target.

There appears to be a refleak of some sort still, since "core stop now"
after performing a transfer results in a delay.


Modified:
    team/mmichelson/atxfer_features/main/bridging_basic.c

Modified: team/mmichelson/atxfer_features/main/bridging_basic.c
URL: http://svnview.digium.com/svn/asterisk/team/mmichelson/atxfer_features/main/bridging_basic.c?view=diff&rev=392917&r1=392916&r2=392917
==============================================================================
--- team/mmichelson/atxfer_features/main/bridging_basic.c (original)
+++ team/mmichelson/atxfer_features/main/bridging_basic.c Tue Jun 25 18:05:24 2013
@@ -31,12 +31,13 @@
 
 ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
 
+#define REF_DEBUG 1
+#include "asterisk/astobj2.h"
 #include "asterisk/channel.h"
 #include "asterisk/utils.h"
 #include "asterisk/linkedlists.h"
 #include "asterisk/bridging.h"
 #include "asterisk/bridging_basic.h"
-#include "asterisk/astobj2.h"
 #include "asterisk/features_config.h"
 #include "asterisk/pbx.h"
 #include "asterisk/file.h"
@@ -142,7 +143,6 @@
  */
 static int bridge_personality_normal_push(struct ast_bridge *self, struct ast_bridge_channel *bridge_channel, struct ast_bridge_channel *swap)
 {
-	ast_log(LOG_NOTICE, "NORMAL PUSH\n");
 	if (ast_bridge_hangup_hook(bridge_channel->features, basic_hangup_hook, NULL, NULL, AST_BRIDGE_HOOK_REMOVE_ON_PULL)
 		|| ast_bridge_channel_setup_features(bridge_channel)) {
 		return -1;
@@ -164,6 +164,17 @@
 	}
 
 	return ast_bridge_base_v_table.push(self, bridge_channel, swap);
+}
+
+static void bridge_basic_destroy(struct ast_bridge *self)
+{
+	struct bridge_basic_personality *personality = self->personality;
+
+	ast_assert(personality != NULL);
+
+	ao2_cleanup(personality);
+
+	ast_bridge_base_v_table.destroy(self);
 }
 
 static void remove_hooks_on_personality_change(struct ast_bridge *bridge)
@@ -388,6 +399,8 @@
 {
 	struct attended_transfer_properties *props = obj;
 
+	ast_log(LOG_NOTICE, "What about now? Can I be a happy man again?\n");
+
 	ao2_cleanup(props->target_bridge);
 	ao2_cleanup(props->transferee_bridge);
 	/* Use ao2_cleanup() instead of ast_channel_unref() for channels since they may be NULL */
@@ -434,18 +447,25 @@
 	return props;
 }
 
-static void attended_transfer_properties_shutdown(struct attended_transfer_properties *props,
-		int hangup_target)
+static void clear_stimulus_queue(struct attended_transfer_properties *props)
+{
+	struct stimulus_list *list;
+	SCOPED_MUTEX(lock, &props->lock);
+
+	while ((list = AST_LIST_REMOVE_HEAD(&props->stimulus_queue, next))) {
+		ast_free(list);
+	}
+}
+
+static void attended_transfer_properties_shutdown(struct attended_transfer_properties *props)
 {
 	if (props->transferee_bridge) {
 		ast_bridge_merge_inhibit(props->transferee_bridge, -1);
+		ast_bridge_basic_change_personality_normal(props->transferee_bridge);
 	}
 
 	if (props->transfer_target) {
 		SCOPED_CHANNELLOCK(lock, props->transfer_target);
-		if (hangup_target) {
-			ast_softhangup_nolock(props->transfer_target, AST_SOFTHANGUP_EXPLICIT);
-		}
 
 		if (props->target_framehook_id != -1) {
 			ast_framehook_detach(props->transfer_target, props->target_framehook_id);
@@ -454,10 +474,15 @@
 
 	if (props->target_bridge) {
 		ast_bridge_destroy(props->target_bridge);
-	}
-	/* XXX There will be more here, such as removing transferer role
-	 * from transferer
-	 */
+		props->target_bridge = NULL;
+	}
+
+	if (props->transferer) {
+		ast_channel_remove_bridge_role(props->transferer, TRANSFERER_ROLE_NAME);
+	}
+
+	clear_stimulus_queue(props);
+
 	ao2_cleanup(props);
 }
 
@@ -502,7 +527,7 @@
 static int bridge_move(struct ast_bridge *dest, struct ast_bridge *src, struct ast_channel *channel, struct ast_channel *swap)
 {
 	int res;
-	struct ast_bridge_channel *bridge_channel;
+	RAII_VAR(struct ast_bridge_channel *, bridge_channel, NULL, ao2_cleanup);
 	SCOPED_LOCK(lock, dest, ast_bridge_lock, ast_bridge_unlock);
 
 	ast_channel_lock(channel);
@@ -521,23 +546,25 @@
 
 static void bridge_merge(struct ast_bridge *dest, struct ast_bridge *src, struct ast_channel **kick_me, unsigned int num_kick)
 {
-	struct ast_bridge_channel **kick_bridge_channels = NULL;
+	struct ast_bridge_channel **kick_bridge_channels = num_kick ?
+		ast_alloca(num_kick * sizeof(*kick_bridge_channels)) : NULL;
 	int i;
 
 	ast_bridge_lock_both(dest, src);
 
-	if (num_kick) {
-		kick_bridge_channels = ast_alloca(num_kick * sizeof(*kick_bridge_channels));
-		for (i = 0; i < num_kick; ++i) {
-			ast_channel_lock(kick_me[i]);
-			kick_bridge_channels[i] = ast_channel_get_bridge_channel(kick_me[i]);
-			ast_channel_unlock(kick_me[i]);
-		}
+	for (i = 0; i < num_kick; ++i) {
+		ast_channel_lock(kick_me[i]);
+		kick_bridge_channels[i] = ast_channel_get_bridge_channel(kick_me[i]);
+		ast_channel_unlock(kick_me[i]);
 	}
 
 	bridge_merge_do(dest, src, kick_bridge_channels, num_kick);
 	ast_bridge_unlock(dest);
 	ast_bridge_unlock(src);
+
+	for (i = 0; i < num_kick; ++i) {
+		ao2_cleanup(kick_bridge_channels[i]);
+	}
 }
 
 /*!
@@ -1027,10 +1054,7 @@
 	const char *swap_dtmf;
 	struct bridge_basic_personality *personality = self->personality;
 
-	ast_log(LOG_NOTICE, "ATXFER PUSH\n");
-
 	if (!ast_channel_has_role(bridge_channel->chan, TRANSFERER_ROLE_NAME)) {
-		ast_log(LOG_NOTICE, "NOT THE TRANSFERER!\n");
 		return 0;
 	}
 
@@ -1097,7 +1121,6 @@
 static void *attended_transfer_monitor_thread(void *data)
 {
 	struct attended_transfer_properties *props = data;
-	ast_log(LOG_NOTICE, "ATXFER MONITOR THREAD START\n");
 
 	for (;;) {
 		enum attended_transfer_stimulus stimulus;
@@ -1125,6 +1148,8 @@
 
 		ast_log(LOG_NOTICE, "Told to enter state %s next\n", state_properties[props->state].state_name);
 	}
+
+	attended_transfer_properties_shutdown(props);
 
 	return NULL;
 }
@@ -1198,7 +1223,7 @@
 
 	if (add_transferer_role(props->transferer, attended_transfer)) {
 		ast_log(LOG_ERROR, "Unable to set transferer bridge role properly\n");
-		attended_transfer_properties_shutdown(props, 0);
+		attended_transfer_properties_shutdown(props);
 		return 0;
 	}
 
@@ -1209,7 +1234,7 @@
 	if (grab_transfer(bridge_channel->chan, props->exten, sizeof(props->exten), props->context)) {
 		ast_log(LOG_WARNING, "Unable to acquire target extension for attended transfer\n");
 		ast_bridge_channel_write_unhold(bridge_channel);
-		attended_transfer_properties_shutdown(props, 0);
+		attended_transfer_properties_shutdown(props);
 		return 0;
 	}
 
@@ -1224,9 +1249,15 @@
 		ast_log(LOG_ERROR, "Unable to request outbound channel for attended transfer target\n");
 		ast_stream_and_wait(props->transferer, props->failsound, AST_DIGIT_NONE);
 		ast_bridge_channel_write_unhold(bridge_channel);
-		attended_transfer_properties_shutdown(props, 0);
+		attended_transfer_properties_shutdown(props);
 		return 0;
 	}
+
+	/* We increase the refcount of the transfer target because ast_bridge_impart() will
+	 * steal the reference we already have. We need to keep a reference, so the only
+	 * choice is to give it a bump
+	 */
+	ast_channel_ref(props->transfer_target);
 
 	/* Create a bridge to use to talk to the person we are calling */
 	props->target_bridge = ast_bridge_basic_new();
@@ -1234,7 +1265,9 @@
 		ast_log(LOG_ERROR, "Unable to create bridge for attended transfer target\n");
 		ast_stream_and_wait(props->transferer, props->failsound, AST_DIGIT_NONE);
 		ast_bridge_channel_write_unhold(bridge_channel);
-		attended_transfer_properties_shutdown(props, 1);
+		ast_hangup(props->transfer_target);
+		props->transfer_target = NULL;
+		attended_transfer_properties_shutdown(props);
 		return 0;
 	}
 	ast_bridge_merge_inhibit(props->target_bridge, +1);
@@ -1243,7 +1276,9 @@
 		ast_log(LOG_ERROR, "Unable to attach framehook to transfer target\n");
 		ast_stream_and_wait(props->transferer, props->failsound, AST_DIGIT_NONE);
 		ast_bridge_channel_write_unhold(bridge_channel);
-		attended_transfer_properties_shutdown(props, 1);
+		ast_hangup(props->transfer_target);
+		props->transfer_target = NULL;
+		attended_transfer_properties_shutdown(props);
 		return 0;
 	}
 
@@ -1254,16 +1289,23 @@
 		ast_log(LOG_ERROR, "Unable to place outbound call to transfer target\n");
 		ast_stream_and_wait(bridge_channel->chan, props->failsound, AST_DIGIT_NONE);
 		ast_bridge_channel_write_unhold(bridge_channel);
-		attended_transfer_properties_shutdown(props, 1);
+		ast_hangup(props->transfer_target);
+		props->transfer_target = NULL;
+		attended_transfer_properties_shutdown(props);
 		return 0;
 	}
 
-	/* This is how this is going down, we are imparting the channel we called above into this bridge first */
+	/* We increase the refcount of the transfer target because ast_bridge_impart() will
+	 * steal the reference we already have. We need to keep a reference, so the only
+	 * choice is to give it a bump
+	 */
+	ast_channel_ref(props->transfer_target);
 	if (ast_bridge_impart(props->target_bridge, props->transfer_target, NULL, NULL, 1)) {
 		ast_log(LOG_ERROR, "Unable to place transfer target into bridge\n");
 		ast_stream_and_wait(bridge_channel->chan, props->failsound, AST_DIGIT_NONE);
 		ast_bridge_channel_write_unhold(bridge_channel);
-		attended_transfer_properties_shutdown(props, 1);
+		ast_hangup(props->transfer_target);
+		attended_transfer_properties_shutdown(props);
 		return 0;
 	}
 
@@ -1271,7 +1313,7 @@
 		ast_log(LOG_ERROR, "Unable to create monitoring thread for attended transfer\n");
 		ast_stream_and_wait(bridge_channel->chan, props->failsound, AST_DIGIT_NONE);
 		ast_bridge_channel_write_unhold(bridge_channel);
-		attended_transfer_properties_shutdown(props, 1);
+		attended_transfer_properties_shutdown(props);
 		return 0;
 	}
 
@@ -1373,6 +1415,7 @@
 	ast_bridge_basic_v_table = ast_bridge_base_v_table;
 	ast_bridge_basic_v_table.name = "basic";
 	ast_bridge_basic_v_table.push = bridge_basic_push;
+	ast_bridge_basic_v_table.destroy = bridge_basic_destroy;
 
 	personality_normal_v_table = ast_bridge_base_v_table;
 	personality_normal_v_table.name = "normal";




More information about the asterisk-commits mailing list