Fix several problems with ast_bridge_add_channel().
authorRichard Mudgett <rmudgett@digium.com>
Wed, 26 Jun 2013 14:38:57 +0000 (14:38 +0000)
committerRichard Mudgett <rmudgett@digium.com>
Wed, 26 Jun 2013 14:38:57 +0000 (14:38 +0000)
* 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.

git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@392953 65c4cc65-6c06-0410-ace0-fbb531ad65f3

main/bridging.c

index b0c2796..a4a1339 100644 (file)
@@ -528,65 +528,80 @@ static void bridge_dissolve(struct ast_bridge *bridge)
 
 /*!
  * \internal
- * \brief Determine whether a bridge channel leaving the bridge will cause it to dissolve or not.
+ * \brief Check if a bridge should dissolve and do it.
  * \since 12.0.0
  *
- * \param bridge_channel Channel causing the check
- * \param bridge The bridge we are concerned with
+ * \param bridge_channel Channel causing the check.
  *
- * \note the bridge should be locked prior to calling this function
+ * \note On entry, bridge_channel->bridge is already locked.
  *
- * \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
+ * \return Nothing
  */
-static int bridge_check_will_dissolve(struct ast_bridge_channel *bridge_channel, struct ast_bridge *bridge, int assume_end_state)
+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;
+               bridge_dissolve(bridge);
+               return;
        }
 
-       switch (assume_end_state ? AST_BRIDGE_CHANNEL_STATE_END : bridge_channel->state) {
+       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;
+/* BUGBUG need to implement AST_BRIDGE_CHANNEL_FLAG_LONELY support here */
 }
 
 /*!
  * \internal
- * \brief Check if a bridge should dissolve and do it.
+ * \brief Check if a bridge should dissolve because of a stolen channel and do it.
  * \since 12.0.0
  *
- * \param bridge_channel Channel causing the check.
+ * \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_channel->bridge is already locked.
+ * \note On entry, bridge and bridge_channel->bridge are already locked.
  *
  * \return Nothing
  */
-static void bridge_dissolve_check(struct ast_bridge_channel *bridge_channel)
+static void bridge_dissolve_check_stolen(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel)
 {
-       struct ast_bridge *bridge = bridge_channel->bridge;
+       if (bridge->dissolved) {
+               return;
+       }
 
-       if (bridge_check_will_dissolve(bridge_channel, bridge, 0)) {
+       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_move(struct ast_bridge *dst_bridge, struct ast_bridge *src_bridge
 }
 
 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 @@ int ast_bridge_add_channel(struct ast_bridge *bridge, struct ast_channel *chan,
 
        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_channel_state(yanked_chan) != AST_STATE_UP) {
+                       ast_answer(yanked_chan);
                }
-               if (ast_bridge_impart(bridge, bridge_chan, NULL, features, 1)) {
+               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 @@ int ast_bridge_add_channel(struct ast_bridge *bridge, struct ast_channel *chan,
                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);
                }