<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2511/">https://reviewboard.asterisk.org/r/2511/</a>
</td>
</tr>
</table>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2511/diff/5/?file=38404#file38404line3883" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/bridge_construction/main/bridging.c</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">3841</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * \param chan_bridge</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">3877</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="cm">/*! Bridges cannot allow for a swap optimization to occure */</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">occure?</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2511/diff/5/?file=38404#file38404line3899" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/bridge_construction/main/bridging.c</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int check_swap_optimize_out(struct ast_bridge *chan_bridge,</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static enum bridge_allow_swap bridges_allow_swap_optimization(struct ast_bridge *chan_bridge,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">3850</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt"><span class="hl">int</span></span><span class="hl"> </span><span class="nf"><span class="hl">check</span>_swap_optimiz<span class="hl">e_out</span></span><span class="p">(</span><span class="k">struct</span> <span class="n">ast_bridge</span> <span class="o">*</span><span class="n">chan_bridge</span><span class="p">,</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">3893</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="k"><span class="hl">enum</span></span><span class="hl"> </span><span class="n"><span class="hl">bridge_allow_swap</span></span><span class="hl"> </span><span class="nf"><span class="hl">bridges_allow</span>_swap_optimiz<span class="hl">ation</span></span><span class="p">(</span><span class="k">struct</span> <span class="n">ast_bridge</span> <span class="o">*</span><span class="n">chan_bridge</span><span class="p">,</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">3851</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">struct</span> <span class="n">ast_bridge_channel</span> <span class="o">*</span><span class="n">chan_bridge_channel</span><span class="p">,</span> <span class="k">struct</span> <span class="n">ast_bridge</span> <span class="o">*</span><span class="n">peer_bridge</span><span class="p">,</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">3894</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="k">struct</span> <span class="n">ast_bridge</span> <span class="o">*</span><span class="n">peer_bridge</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">You need to judiciously add checks for AST_BRIDGE_FLAG_TRANSFER_BRIDGE_ONLY that prevents the bridge from being a source of swapped channels. In this function, AST_BRIDGE_FLAG_TRANSFER_BRIDGE_ONLY acts like another AST_BRIDGE_FLAG_SWAP_INHIBIT_FROM flag.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2511/diff/5/?file=38404#file38404line5521" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/bridge_construction/main/bridging.c</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">5493</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * \param source_bridge The bridge from which a channel is moved</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The source_bridge parameter is not used.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2511/diff/5/?file=38404#file38404line5533" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/bridge_construction/main/bridging.c</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">5505</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">bridged_to_source</span> <span class="o">&&</span> <span class="n">bridged_to_source</span><span class="o">-></span><span class="n">state</span> <span class="o">==</span> <span class="n">AST_BRIDGE_CHANNEL_STATE_WAIT</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Another check is needed:
Check the bridged_to_source->features AST_BRIDGE_CHANNEL_FLAG_IMMOVABLE flag. Though, I'm not real sure that this flag will be present when you are going to move/swap because the immovable flag could imply that the bridge must be transferred.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2511/diff/5/?file=38404#file38404line5583" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/bridge_construction/main/bridging.c</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">5555</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">case</span> <span class="n">AST_BRIDGE_OPTIMIZE_PROHIBITED</span>:</pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">5556</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="nl">default:</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">5557</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="k">return</span> <span class="n">attended_transfer_bridge</span><span class="p">(</span><span class="n">to_transferee</span><span class="p">,</span> <span class="n">to_transfer_target</span><span class="p">,</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">5558</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">to_transferee_bridge</span><span class="p">,</span> <span class="n">to_target_bridge</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This handles the case of a multi-party bridge to a parking bridge. However, what happens when the user transfers a call from one holding bridge to another holding bridge? This could come about if one call is in a queue holding bridge and the user then transfers the call to park.
You will also get here for the temporary bridge->inhibit_merge condition or bridge->dissolved. Should the transfer be invalid in these cases?</pre>
</div>
<br />
<p>- rmudgett</p>
<br />
<p>On May 21st, 2013, 9:23 p.m. UTC, Mark Michelson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers and rmudgett.</div>
<div>By Mark Michelson.</div>
<p style="color: grey;"><i>Updated May 21, 2013, 9:23 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-21334">ASTERISK-21334</a>,
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-21336">ASTERISK-21336</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This changeset contains two separate items
First, there is a new bridge function called ast_bridge_transfer_attended(). In review 2470, this was a stubbed out function, but here, the actual content has been filled in. It performs an attended transfer given the two transferer channels involved. This function is intended to handle cases where both transferer channels are in bridges, or where only one transferer channel is in a bridge. There is currently one piece of functionality missing, and it is dependent on review 2508. I cannot currently perform an attended transfer of a bridge to an unbridged call since I cannot manipulate a local channel in the appropriate way. The addition of the unreal channels in 2508 will allow me to do this.
Second, the larger of the changes was to go through places where masquerades are performed and eliminate or hide them as much as possible. Two new functions have been created in the channel API, ast_channel_yank() and ast_channel_move(). ast_channel_yank() is intended to be used when code needs to gain local control of a channel in order to redirect it someplace new. ast_channel_move() is intended to be used as a replacement for old uses of ast_channel_masquerade() and ast_do_masquerade() in order to move one channel into where another channel is executing. Neither of these functions should be needed on bridged channels since the bridging API provides ways for channels to move between bridges or perform a task upon exiting a bridge.
The highlights of this second change include changing the Bridge AMI action and Bridge dialplan applications to no longer require explicit masquerade calls in order to operate. Similarly, the masquerade call made in ast_do_pickup() has changed to use ast_channel_move() instead. ast_async_goto() has been modified to use ast_channel_yank() when redirecting an unbridged channel.
In the summary for the review, I mentioned that masquerade usage is only partially handled. There are some road blocks preventing complete masquerade hiding:
1) The blind transfer and attended transfer code in features.c currently uses masquerades. This code is more-or-less dead at this point, but since the code is still in the tree, it's still using masquerade calls. This code is slated for eventual removal.
2) There are a number of channel drivers that make use of ast_channel_masquerade() and ast_channel_transfer_masquerade() in order to perform transfers. These are slated to eventually make use of ast_bridge_transfer_blind() and ast_bridge_transfer_attended() in order to actually perform their transfers.
3) There are several channel API calls that check ast_channel_masq(chan) and will call ast_do_masquerade() if the result is non-NULL. Unfortunately, this is still needed because of the channel drivers mentioned in item 2. They simply set up the masquerade and expect the channel thread to actually perform the masquerade.
Once these three points are addressed, then ast_channel_transfer_masquerade() can be removed entirely, and ast_channel_masquerade() and ast_do_masquerade() can be combined into a single function that simply performs the masquerade.
As a final note, I also, because I was thinking about these sorts of things, changed the bridge builtin blind transfer code to use ast_bridge_transfer_blind() instead of using what it was previously using.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I tested the Manager Bridge action and the Bridge dialplan application with these changes applied. I also did manager redirects of bridged and unbridged channels to ensure they were redirected properly.
I tested attended transfer code using chan_sip. The chan_sip changes will be in a separate review.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/team/group/bridge_construction/CHANGES <span style="color: grey">(389354)</span></li>
<li>/team/group/bridge_construction/bridges/bridge_builtin_features.c <span style="color: grey">(389354)</span></li>
<li>/team/group/bridge_construction/channels/chan_mgcp.c <span style="color: grey">(389354)</span></li>
<li>/team/group/bridge_construction/channels/chan_sip.c <span style="color: grey">(389354)</span></li>
<li>/team/group/bridge_construction/include/asterisk/bridging.h <span style="color: grey">(389354)</span></li>
<li>/team/group/bridge_construction/include/asterisk/channel.h <span style="color: grey">(389354)</span></li>
<li>/team/group/bridge_construction/main/bridging.c <span style="color: grey">(389354)</span></li>
<li>/team/group/bridge_construction/main/channel.c <span style="color: grey">(389354)</span></li>
<li>/team/group/bridge_construction/main/features.c <span style="color: grey">(389354)</span></li>
<li>/team/group/bridge_construction/main/pbx.c <span style="color: grey">(389354)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2511/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>