[asterisk-commits] Bridge core: Pass a ref with the swap channel when joining a... (asterisk[certified/13.1])
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Fri Apr 22 17:21:32 CDT 2016
Joshua Colp has submitted this change and it was merged.
Change subject: Bridge core: Pass a ref with the swap channel when joining a bridge.
......................................................................
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/
This patch is a remedial cherry-pick from v13.
Change-Id: I73fdf13a3a1042566281c7d06d6e83e2ef87c120
---
M include/asterisk/bridge.h
M include/asterisk/bridge_channel_internal.h
M include/asterisk/bridge_internal.h
M main/bridge.c
M main/bridge_channel.c
5 files changed, 54 insertions(+), 11 deletions(-)
Approvals:
Mark Michelson: Looks good to me, approved
Joshua Colp: Looks good to me, but someone else must approve; Verified
diff --git a/include/asterisk/bridge.h b/include/asterisk/bridge.h
index 0616f0e..e5d4820 100644
--- a/include/asterisk/bridge.h
+++ b/include/asterisk/bridge.h
@@ -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.)
diff --git a/include/asterisk/bridge_channel_internal.h b/include/asterisk/bridge_channel_internal.h
index b5a3a8d..71892f5 100644
--- a/include/asterisk/bridge_channel_internal.h
+++ b/include/asterisk/bridge_channel_internal.h
@@ -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);
diff --git a/include/asterisk/bridge_internal.h b/include/asterisk/bridge_internal.h
index e50e7f9..37c8418 100644
--- a/include/asterisk/bridge_internal.h
+++ b/include/asterisk/bridge_internal.h
@@ -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.
diff --git a/main/bridge.c b/main/bridge.c
index 3a01b58..bd5f63c 100644
--- a/main/bridge.c
+++ b/main/bridge.c
@@ -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;
}
diff --git a/main/bridge_channel.c b/main/bridge_channel.c
index d6c1048..ab358dd 100644
--- a/main/bridge_channel.c
+++ b/main/bridge_channel.c
@@ -2530,11 +2530,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,
@@ -2578,6 +2578,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;
@@ -2603,6 +2606,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) {
@@ -2623,6 +2631,9 @@
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) {
ast_debug(1, "Channel %s simulating UNHOLD for bridge end.\n",
--
To view, visit https://gerrit.asterisk.org/2663
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I73fdf13a3a1042566281c7d06d6e83e2ef87c120
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: certified/13.1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
More information about the asterisk-commits
mailing list