Author: rmudgett Date: Thu Jan 22 13:24:28 2015 New Revision: 430975 URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=430975 Log: Bridge core: Pass a ref with the swap channel when joining a bridge.
When code imparts a channel into a bridge to swap with another channel, a ref needs to be held on the swap channel to ensure that it cannot dissapear before finding it in the bridge. * The ast_bridge_join() swap channel parameter now always steals a ref for the swap channel. This is the only change to the bridge framework's public API semantics. * bridge_channel_internal_join() now requires the bridge_channel->swap channel to pass in a ref. ASTERISK-24649 Reported by: John Bigelow Review: https://reviewboard.asterisk.org/r/4354/ Modified: branches/13/include/asterisk/bridge.h branches/13/include/asterisk/bridge_channel_internal.h branches/13/include/asterisk/bridge_internal.h branches/13/main/bridge.c branches/13/main/bridge_channel.c Modified: branches/13/include/asterisk/bridge.h URL: http://svnview.digium.com/svn/asterisk/branches/13/include/asterisk/bridge.h?view=diff&rev=430975&r1=430974&r2=430975 ============================================================================== --- branches/13/include/asterisk/bridge.h (original) +++ branches/13/include/asterisk/bridge.h Thu Jan 22 13:24:28 2015 @@ -444,14 +444,18 @@ }; /*! - * \brief Join (blocking) a channel to a bridge + * \brief Join a channel to a bridge (blocking) * * \param bridge Bridge to join * \param chan Channel to join - * \param swap Channel to swap out if swapping + * \param swap Channel to swap out if swapping (A channel reference is stolen.) * \param features Bridge features structure * \param tech_args Optional Bridging tech optimization parameters for this channel. * \param flags defined by enum ast_bridge_join_flags. + * + * \note The passed in swap channel is always unreffed on return. It is not a + * good idea to access the swap channel on return or for the caller to keep a + * reference to it. * * \note Absolutely _NO_ locks should be held before calling * this function since it blocks. @@ -495,7 +499,7 @@ }; /*! - * \brief Impart (non-blocking) a channel onto a bridge + * \brief Impart a channel to a bridge (non-blocking) * * \param bridge Bridge to impart on * \param chan Channel to impart (The channel reference is stolen if impart successful.) Modified: branches/13/include/asterisk/bridge_channel_internal.h URL: http://svnview.digium.com/svn/asterisk/branches/13/include/asterisk/bridge_channel_internal.h?view=diff&rev=430975&r1=430974&r2=430975 ============================================================================== --- branches/13/include/asterisk/bridge_channel_internal.h (original) +++ branches/13/include/asterisk/bridge_channel_internal.h Thu Jan 22 13:24:28 2015 @@ -103,6 +103,9 @@ * * \param bridge_channel Channel to push. * + * \note A ref is not held by bridge_channel->swap when calling because the + * push with swap happens immediately. + * * \note On entry, bridge_channel->bridge is already locked. * * \retval 0 on success. @@ -128,16 +131,22 @@ /*! * \internal - * \brief Join the bridge_channel to the bridge + * \brief Join the bridge_channel to the bridge (blocking) * * \param bridge_channel The Channel in the bridge + * + * \note The bridge_channel->swap holds a channel reference for the swap + * channel going into the bridging system. The ref ensures that the swap + * pointer is valid for the bridge subclass push callbacks. The pointer + * will be NULL on return if the ref was consumed. + * + * \details + * This API call puts the bridge_channel into the bridge and handles the + * bridge_channel's processing of events while it is in the bridge. It + * will return when the channel has been instructed to leave the bridge. * * \retval 0 bridge channel successfully joined the bridge * \retval -1 bridge channel failed to join the bridge - * - * \note This API call starts the bridge_channel's processing of events while - * it is in the bridge. It will return when the channel has been instructed to - * leave the bridge. */ int bridge_channel_internal_join(struct ast_bridge_channel *bridge_channel); Modified: branches/13/include/asterisk/bridge_internal.h URL: http://svnview.digium.com/svn/asterisk/branches/13/include/asterisk/bridge_internal.h?view=diff&rev=430975&r1=430974&r2=430975 ============================================================================== --- branches/13/include/asterisk/bridge_internal.h (original) +++ branches/13/include/asterisk/bridge_internal.h Thu Jan 22 13:24:28 2015 @@ -117,6 +117,9 @@ * \param attempt_recovery TRUE if failure attempts to push channel back into original bridge. * \param optimized Indicates whether the move is part of an unreal channel optimization. * + * \note A ref is not held by bridge_channel->swap when calling because the + * move with swap happens immediately. + * * \note The dst_bridge and bridge_channel->bridge are assumed already locked. * * \retval 0 on success. Modified: branches/13/main/bridge.c URL: http://svnview.digium.com/svn/asterisk/branches/13/main/bridge.c?view=diff&rev=430975&r1=430974&r2=430975 ============================================================================== --- branches/13/main/bridge.c (original) +++ branches/13/main/bridge.c Thu Jan 22 13:24:28 2015 @@ -1560,6 +1560,7 @@ ao2_ref(bridge, -1); } if (!bridge_channel) { + ao2_t_cleanup(swap, "Error exit: bridge_channel alloc failed"); res = -1; goto join_exit; } @@ -1567,6 +1568,7 @@ ast_assert(features != NULL); if (!features) { ao2_ref(bridge_channel, -1); + ao2_t_cleanup(swap, "Error exit: features is NULL"); res = -1; goto join_exit; } @@ -1596,6 +1598,8 @@ ast_channel_internal_bridge_channel_set(chan, NULL); ast_channel_unlock(chan); bridge_channel->chan = NULL; + /* If bridge_channel->swap is not NULL then the join failed. */ + ao2_t_cleanup(bridge_channel->swap, "Bridge complete: join failed"); bridge_channel->swap = NULL; bridge_channel->features = NULL; @@ -1624,7 +1628,12 @@ bridge_channel_internal_join(bridge_channel); - /* cleanup */ + /* + * cleanup + * + * If bridge_channel->swap is not NULL then the join failed. + */ + ao2_t_cleanup(bridge_channel->swap, "Bridge complete: Departable impart join failed"); bridge_channel->swap = NULL; ast_bridge_features_destroy(bridge_channel->features); bridge_channel->features = NULL; @@ -1653,6 +1662,8 @@ ast_channel_internal_bridge_channel_set(chan, NULL); ast_channel_unlock(chan); bridge_channel->chan = NULL; + /* If bridge_channel->swap is not NULL then the join failed. */ + ao2_t_cleanup(bridge_channel->swap, "Bridge complete: Independent impart join failed"); bridge_channel->swap = NULL; ast_bridge_features_destroy(bridge_channel->features); bridge_channel->features = NULL; @@ -1706,7 +1717,7 @@ } ast_channel_unlock(chan); bridge_channel->chan = chan; - bridge_channel->swap = swap; + bridge_channel->swap = ao2_t_bump(swap, "Setting up bridge impart"); bridge_channel->features = features; bridge_channel->inhibit_colp = !!(flags & AST_BRIDGE_IMPART_INHIBIT_JOIN_COLP); bridge_channel->depart_wait = @@ -1730,6 +1741,7 @@ ast_channel_internal_bridge_channel_set(chan, NULL); ast_channel_unlock(chan); bridge_channel->chan = NULL; + ao2_t_cleanup(bridge_channel->swap, "Bridge complete: Impart failed"); bridge_channel->swap = NULL; ast_bridge_features_destroy(bridge_channel->features); bridge_channel->features = NULL; @@ -2171,7 +2183,11 @@ /* * The channel died as a result of being pulled. Leave it * pointing to the original bridge. + * + * Clear out the swap channel pointer. A ref is not held + * by bridge_channel->swap at this point. */ + bridge_channel->swap = NULL; bridge_reconfigured(orig_bridge, 0); return -1; } Modified: branches/13/main/bridge_channel.c URL: http://svnview.digium.com/svn/asterisk/branches/13/main/bridge_channel.c?view=diff&rev=430975&r1=430974&r2=430975 ============================================================================== --- branches/13/main/bridge_channel.c (original) +++ branches/13/main/bridge_channel.c Thu Jan 22 13:24:28 2015 @@ -2490,11 +2490,11 @@ ao2_iterator_destroy(&iter); } -/*! \brief Join a channel to a bridge and handle anything the bridge may want us to do */ int bridge_channel_internal_join(struct ast_bridge_channel *bridge_channel) { int res = 0; struct ast_bridge_features *channel_features; + struct ast_channel *swap; ast_debug(1, "Bridge %s: %p(%s) is joining\n", bridge_channel->bridge->uniqueid, @@ -2538,6 +2538,9 @@ bridge_channel->bridge->callid = ast_read_threadstorage_callid(); } + /* Take the swap channel ref from the bridge_channel struct. */ + swap = bridge_channel->swap; + if (bridge_channel_internal_push(bridge_channel)) { int cause = bridge_channel->bridge->cause; @@ -2563,6 +2566,11 @@ } ast_bridge_unlock(bridge_channel->bridge); + + /* Must release any swap ref after unlocking the bridge. */ + ao2_t_cleanup(swap, "Bridge push with swap successful"); + swap = NULL; + bridge_channel_event_join_leave(bridge_channel, AST_BRIDGE_HOOK_TYPE_JOIN); while (bridge_channel->state == BRIDGE_CHANNEL_STATE_WAIT) { @@ -2582,6 +2590,9 @@ bridge_reconfigured(bridge_channel->bridge, 1); ast_bridge_unlock(bridge_channel->bridge); + + /* Must release any swap ref after unlocking the bridge. */ + ao2_t_cleanup(swap, "Bridge push with swap failed or exited immediately"); /* Complete any active hold before exiting the bridge. */ if (ast_channel_hold_state(bridge_channel->chan) == AST_CONTROL_HOLD) { -- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- svn-commits mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/svn-commits