[asterisk-commits] rmudgett: trunk r392953 - /trunk/main/bridging.c
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Wed Jun 26 09:38:59 CDT 2013
Author: rmudgett
Date: Wed Jun 26 09:38:57 2013
New Revision: 392953
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=392953
Log:
Fix several problems with ast_bridge_add_channel().
* Fix locking problems. ast_bridge_move() locks two bridges. To do that,
deadlock avoidance must be done. Called bridge_move_locked() instead.
* Fix inconsistency in the bridge dissolve check callers. The original
caller has already removed the channel from the bridge. The new caller
has not removed the channel from the bridge. Reverted
bridge_dissolve_check() and added bridge_dissolve_check_stolen() to be
used by the new caller on the original bridge after the channel is moved
to the new bridge.
* Fix memory leak of features if the added channel was already in a
bridge.
* Fix incorrect call to ast_bridge_impart().
* Renamed bridge_chan to yanked_chan.
Modified:
trunk/main/bridging.c
Modified: trunk/main/bridging.c
URL: http://svnview.digium.com/svn/asterisk/trunk/main/bridging.c?view=diff&rev=392953&r1=392952&r2=392953
==============================================================================
--- trunk/main/bridging.c (original)
+++ trunk/main/bridging.c Wed Jun 26 09:38:57 2013
@@ -528,65 +528,80 @@
/*!
* \internal
- * \brief Determine whether a bridge channel leaving the bridge will cause it to dissolve or not.
- * \since 12.0.0
- *
- * \param bridge_channel Channel causing the check
- * \param bridge The bridge we are concerned with
- *
- * \note the bridge should be locked prior to calling this function
- *
- * \retval 0 if the channel leaving shouldn't cause the bridge to dissolve
- * \retval non-zero if the channel should cause the bridge to dissolve
- */
-static int bridge_check_will_dissolve(struct ast_bridge_channel *bridge_channel, struct ast_bridge *bridge, int assume_end_state)
-{
+ * \brief Check if a bridge should dissolve and do it.
+ * \since 12.0.0
+ *
+ * \param bridge_channel Channel causing the check.
+ *
+ * \note On entry, bridge_channel->bridge is already locked.
+ *
+ * \return Nothing
+ */
+static void bridge_dissolve_check(struct ast_bridge_channel *bridge_channel)
+{
+ struct ast_bridge *bridge = bridge_channel->bridge;
+
if (bridge->dissolved) {
- /* The bridge is already dissolved. Don't try to dissolve it again. */
- return 0;
+ return;
}
if (!bridge->num_channels
&& ast_test_flag(&bridge->feature_flags, AST_BRIDGE_FLAG_DISSOLVE_EMPTY)) {
/* Last channel leaving the bridge turns off the lights. */
- return 1;
- }
-
- switch (assume_end_state ? AST_BRIDGE_CHANNEL_STATE_END : bridge_channel->state) {
+ bridge_dissolve(bridge);
+ return;
+ }
+
+ switch (bridge_channel->state) {
case AST_BRIDGE_CHANNEL_STATE_END:
/* Do we need to dissolve the bridge because this channel hung up? */
if (ast_test_flag(&bridge->feature_flags, AST_BRIDGE_FLAG_DISSOLVE_HANGUP)
|| (bridge_channel->features->usable
&& ast_test_flag(&bridge_channel->features->feature_flags,
AST_BRIDGE_CHANNEL_FLAG_DISSOLVE_HANGUP))) {
- return 1;
- }
-
+ bridge_dissolve(bridge);
+ return;
+ }
break;
default:
break;
}
- /* BUGBUG need to implement AST_BRIDGE_CHANNEL_FLAG_LONELY support here */
- return 0;
-}
-
-/*!
- * \internal
- * \brief Check if a bridge should dissolve and do it.
- * \since 12.0.0
- *
- * \param bridge_channel Channel causing the check.
- *
- * \note On entry, bridge_channel->bridge is already locked.
+/* BUGBUG need to implement AST_BRIDGE_CHANNEL_FLAG_LONELY support here */
+}
+
+/*!
+ * \internal
+ * \brief Check if a bridge should dissolve because of a stolen channel and do it.
+ * \since 12.0.0
+ *
+ * \param bridge Bridge to check.
+ * \param bridge_channel Stolen channel causing the check. It is not in the bridge to check and may be in another bridge.
+ *
+ * \note On entry, bridge and bridge_channel->bridge are already locked.
*
* \return Nothing
*/
-static void bridge_dissolve_check(struct ast_bridge_channel *bridge_channel)
-{
- struct ast_bridge *bridge = bridge_channel->bridge;
-
- if (bridge_check_will_dissolve(bridge_channel, bridge, 0)) {
+static void bridge_dissolve_check_stolen(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel)
+{
+ if (bridge->dissolved) {
+ return;
+ }
+
+ if (bridge_channel->features->usable
+ && ast_test_flag(&bridge_channel->features->feature_flags,
+ AST_BRIDGE_CHANNEL_FLAG_DISSOLVE_HANGUP)) {
+ /* The stolen channel controlled the bridge it was stolen from. */
bridge_dissolve(bridge);
+ return;
+ }
+ if (bridge->num_channels < 2
+ && ast_test_flag(&bridge->feature_flags, AST_BRIDGE_FLAG_DISSOLVE_HANGUP)) {
+ /*
+ * The stolen channel has not left enough channels to keep the
+ * bridge alive. Assume the stolen channel hung up.
+ */
+ bridge_dissolve(bridge);
+ return;
}
}
@@ -4322,10 +4337,10 @@
}
int ast_bridge_add_channel(struct ast_bridge *bridge, struct ast_channel *chan,
- struct ast_bridge_features *features, int play_tone, const char *xfersound)
+ struct ast_bridge_features *features, int play_tone, const char *xfersound)
{
RAII_VAR(struct ast_bridge *, chan_bridge, NULL, ao2_cleanup);
- struct ast_channel *bridge_chan = NULL;
+ RAII_VAR(struct ast_channel *, yanked_chan, NULL, ao2_cleanup);
ast_channel_lock(chan);
chan_bridge = ast_channel_get_bridge(chan);
@@ -4333,45 +4348,53 @@
if (chan_bridge) {
RAII_VAR(struct ast_bridge_channel *, bridge_channel, NULL, ao2_cleanup);
- int hangup = 0;
-
- /* Simply moving the channel from the bridge won't perform the dissolve check
- * so we need to manually check here to see if we should dissolve after moving. */
- ao2_lock(chan_bridge);
- if ((bridge_channel = ast_channel_get_bridge_channel(chan))) {
- hangup = bridge_check_will_dissolve(bridge_channel, chan_bridge, 1);
- }
-
- if (ast_bridge_move(bridge, chan_bridge, chan, NULL, 1)) {
- ao2_unlock(chan_bridge);
+
+ ast_bridge_lock_both(bridge, chan_bridge);
+ bridge_channel = find_bridge_channel(chan_bridge, chan);
+ if (bridge_move_locked(bridge, chan_bridge, chan, NULL, 1)) {
+ ast_bridge_unlock(chan_bridge);
+ ast_bridge_unlock(bridge);
return -1;
}
- if (hangup) {
- bridge_dissolve(chan_bridge);
- }
- ao2_unlock(chan_bridge);
-
+ /*
+ * bridge_move_locked() will implicitly ensure that
+ * bridge_channel is not NULL.
+ */
+ ast_assert(bridge_channel != NULL);
+
+ /*
+ * Additional checks if the channel we just stole dissolves the
+ * original bridge.
+ */
+ bridge_dissolve_check_stolen(chan_bridge, bridge_channel);
+ ast_bridge_unlock(chan_bridge);
+ ast_bridge_unlock(bridge);
+
+ /* The channel was in a bridge so it is not getting any new features. */
+ ast_bridge_features_destroy(features);
} else {
/* Slightly less easy case. We need to yank channel A from
* where he currently is and impart him into our bridge.
*/
- bridge_chan = ast_channel_yank(chan);
- if (!bridge_chan) {
+ yanked_chan = ast_channel_yank(chan);
+ if (!yanked_chan) {
ast_log(LOG_WARNING, "Could not gain control of channel %s\n", ast_channel_name(chan));
return -1;
}
- if (ast_channel_state(bridge_chan) != AST_STATE_UP) {
- ast_answer(bridge_chan);
- }
- if (ast_bridge_impart(bridge, bridge_chan, NULL, features, 1)) {
+ if (ast_channel_state(yanked_chan) != AST_STATE_UP) {
+ ast_answer(yanked_chan);
+ }
+ ast_channel_ref(yanked_chan);
+ if (ast_bridge_impart(bridge, yanked_chan, NULL, features, 1)) {
ast_log(LOG_WARNING, "Could not add %s to the bridge\n", ast_channel_name(chan));
+ ast_hangup(yanked_chan);
return -1;
}
}
if (play_tone && !ast_strlen_zero(xfersound)) {
- struct ast_channel *play_chan = bridge_chan ?: chan;
+ struct ast_channel *play_chan = yanked_chan ?: chan;
RAII_VAR(struct ast_bridge_channel *, play_bridge_channel, NULL, ao2_cleanup);
ast_channel_lock(play_chan);
@@ -4379,8 +4402,8 @@
ast_channel_unlock(play_chan);
if (!play_bridge_channel) {
- ast_log(LOG_WARNING, "Unable to play tone for channel %s. Unable to get bridge channel\n",
- ast_channel_name(play_chan));
+ ast_log(LOG_WARNING, "Unable to play tone for channel %s. No longer in a bridge.\n",
+ ast_channel_name(play_chan));
} else {
ast_bridge_channel_queue_playfile(play_bridge_channel, NULL, xfersound, NULL);
}
More information about the asterisk-commits
mailing list