<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/1902/">https://reviewboard.asterisk.org/r/1902/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 17th, 2012, 12:34 p.m., <b>opticron</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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/1902/diff/4/?file=27898#file27898line1061" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/main/bridging.c</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static enum ast_bridge_channel_state bridge_channel_join(struct ast_bridge_channel *bridge_channel)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">968</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">while</span> <span class="p">(</span><span class="n">bridge_channel</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>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1061</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">while</span> <span class="p">(</span><span class="n">bridge_channel</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="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">On two back to back deferreds, this condition will be false and the channel will exit the bridge unexpectedly.</pre>
</blockquote>
<p>On May 17th, 2012, 3:47 p.m., <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">That shouldn't be possible.
When a deferred is actually serviced by this loop, the state (unless the channel is hung up) is set back to AST_BRIDGE_CHANNEL_STATE_WAIT. Each deferred that is serviced can, at most, change the state a single time; only a single deferred request is executed at a time.
Since the deferred no longer changes the bridge_channel->state explicitly but instead requests a state change, executing any number of deferred requests back to back will not break the loop. When a deferred requests a state change, that change is obtained in one of the *_join methods (multithreaded or single), and processed before a new one is pulled off of the queue. The act of processing should either break the loop intentionally due to a hangup, or set the state back to AST_BRIDGE_CHANNEL_STATE_WAIT.
You should be able to queue up n deferred actions and have this process them as it can, and not have any race conditions on the value of state (at least with respect to setting the state value).
</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Nuts. I was thinking through this some more and you're right - the act of putting the channel back into a WAIT needs to come before other state change requests when you're resetting the state.
I'll think about this some more and come up with a mechanism for this.</pre>
<br />
<p>- Matt</p>
<br />
<p>On May 14th, 2012, 9:51 p.m., Matt Jordan wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/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 Joshua Colp.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated May 14, 2012, 9:51 p.m.</i></p>
<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;">Currently, most actions that are executed upon a channel in a conference (increasing/decreasing listening volume, playing back a sound on a channel, etc.) are handled via DTMF menus associated with a user profile. While this suffices for scenarios where a user initiates an action upon themselves, it does not account for scenarios where a third party application wants to interact with the channels in a ConfBridge. While there are a handful of AMI commands that allow for external interaction with the channels in the conference, they are currently limited to a subset of the defined actions.
Rather then add individual AMI commands for each confbridge action, this patch adds a single new AMI command for ConfBridge, ConfbridgeExecAction. The command lets you execute any of the ConfBridge actions on a channel in the conference.
In order to facilitate this, a new mechanism has been added to the bridging layer to allow for a callback function to be executed at the next convenient moment on a bridged channel's thread. This lets a user of the bridging layer defer execution of some function until such a time that the bridging layer determines that it is safe to execute that action on the channel's thread.
Example Usage:
Action: ConfbridgeExecAction
Conference: 1
Channel: Local/blah@foo
Actions: increase_listening_volume,playback(tt-monkeys)
This would playback monkeys to the offending channel. Very loudly.</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;">Initial testing using a test in the Asterisk Test Suite verified proper behavior of the AMI command. The channel was properly suspended from the bridging layer, the playback confbridge action was executed, and the channel was placed back into the bridging layer. All of this was similar to what would occur if the same action was triggered using a DTMF menu.
Note that a test case is being written to handle the various actions, but will be posted under 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>/trunk/CHANGES <span style="color: grey">(366506)</span></li>
<li>/trunk/apps/app_confbridge.c <span style="color: grey">(366506)</span></li>
<li>/trunk/apps/confbridge/conf_config_parser.c <span style="color: grey">(366506)</span></li>
<li>/trunk/apps/confbridge/include/confbridge.h <span style="color: grey">(366506)</span></li>
<li>/trunk/include/asterisk/bridging.h <span style="color: grey">(366506)</span></li>
<li>/trunk/main/bridging.c <span style="color: grey">(366506)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1902/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>