<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/2670/">https://reviewboard.asterisk.org/r/2670/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 17th, 2013, 5:39 p.m. UTC, <b>rmudgett</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/2670/diff/2/?file=41370#file41370line1432" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/include/asterisk/channel.h</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">const struct ast_channel_tech *ast_get_channel_tech(const char *name);</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1432</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">int</span> <span class="nf">ast_hangup</span><span class="p">(</span><span class="k">struct</span> <span class="n">ast_channel</span> <span class="o">*</span><span class="n">chan</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">1432</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">int</span> <span class="nf">ast_hangup</span><span class="p">(</span><span class="k">struct</span> <span class="n">ast_channel</span> <span class="o">*</span><span class="n">chan</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;">This really should be made a void function since this is really a destructor. Noone cares about the return value except some CDR and CEL test code that can easily be changed.</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;">I agree with the sentiment below that this should be a separate issue.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 17th, 2013, 5:39 p.m. UTC, <b>rmudgett</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/2670/diff/2/?file=41375#file41375line2640" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/main/channel.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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 void destroy_hooks(struct ast_channel *chan)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2640</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int ast_hangup(struct ast_channel *chan)</pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2640</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int ast_hangup(struct ast_channel *chan)</pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2641</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">{</pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2641</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">{</pre></td>
</tr>
</tbody>
<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">2642</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span>if (!chan) {</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">2643</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>return -1;</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">2644</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </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;">Since you have made this NULL tolerant, the safe_channel_hangup() in test_stasis_endpoints.c can be removed.
I think the changes to ast_hangup() and the resulting cleanup should be done in a separate patch that this review depends upon.</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;">This review is somewhat time sensitive and there is already another issue that is also time sensitive that is dependant on it, so I don't think making this issue have a further dependency is the right approach right now. I believe we should just file an issue to handle these things and continue as is for now.</pre>
<br />
<p>- jrose</p>
<br />
<p>On July 12th, 2013, 9:23 p.m. UTC, jrose 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, David Lee, kmoore, Matt Jordan, and rmudgett.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated July 12, 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-21592">ASTERISK-21592</a>,
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-21593">ASTERISK-21593</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;">The diff has become somewhat wide reaching, but this review primarily takes care of the following:
1. Provides a new channel driver for creating unreal channels with specific roles already applied to them and a mechanism for shoving the ;2 end of that channel into a bridge
2. Provides Playback and Record on Bridge ARI functions which make use of that channel
In the process of working on this a few bugs got in the way and this review takes care of those as well.
1. Confbridge announcer channels have a reference leak and don't get destroyed on leaving the bridge
2. Numerous ARI function documentation items were flawed and others didn't match the intended implementation and had to be tweaked.</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;">All ARI playback and recording functions on channels were tested to make sure they pass validation after the changes I've made
All ARI playback and recording functions on bridges were tested for functionality in softmix bridges
I also did some rudimentary tests to see that memory wasn't leaking in the files I touched under these normal conditions. It's hardly exhaustive, but it's a start.</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/apps/confbridge/conf_chan_announce.c <span style="color: grey">(394203)</span></li>
<li>/trunk/channels/chan_bridge_media.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/include/asterisk/bridging.h <span style="color: grey">(394203)</span></li>
<li>/trunk/include/asterisk/channel.h <span style="color: grey">(394203)</span></li>
<li>/trunk/include/asterisk/core_unreal.h <span style="color: grey">(394203)</span></li>
<li>/trunk/include/asterisk/stasis_app.h <span style="color: grey">(394203)</span></li>
<li>/trunk/include/asterisk/stasis_app_playback.h <span style="color: grey">(394203)</span></li>
<li>/trunk/main/bridging.c <span style="color: grey">(394203)</span></li>
<li>/trunk/main/channel.c <span style="color: grey">(394203)</span></li>
<li>/trunk/main/core_unreal.c <span style="color: grey">(394203)</span></li>
<li>/trunk/res/res_stasis.c <span style="color: grey">(394203)</span></li>
<li>/trunk/res/res_stasis_http_bridges.c <span style="color: grey">(394203)</span></li>
<li>/trunk/res/res_stasis_http_channels.c <span style="color: grey">(394203)</span></li>
<li>/trunk/res/res_stasis_http_playback.c <span style="color: grey">(394203)</span></li>
<li>/trunk/res/res_stasis_playback.c <span style="color: grey">(394203)</span></li>
<li>/trunk/res/stasis/control.c <span style="color: grey">(394203)</span></li>
<li>/trunk/res/stasis_http/ari_model_validators.h <span style="color: grey">(394203)</span></li>
<li>/trunk/res/stasis_http/ari_model_validators.c <span style="color: grey">(394203)</span></li>
<li>/trunk/res/stasis_http/resource_bridges.h <span style="color: grey">(394203)</span></li>
<li>/trunk/res/stasis_http/resource_bridges.c <span style="color: grey">(394203)</span></li>
<li>/trunk/res/stasis_http/resource_channels.c <span style="color: grey">(394203)</span></li>
<li>/trunk/rest-api/api-docs/bridges.json <span style="color: grey">(394203)</span></li>
<li>/trunk/rest-api/api-docs/channels.json <span style="color: grey">(394203)</span></li>
<li>/trunk/rest-api/api-docs/playback.json <span style="color: grey">(394203)</span></li>
<li>/trunk/rest-api/api-docs/recordings.json <span style="color: grey">(394203)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2670/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>