<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/3625/">https://reviewboard.asterisk.org/r/3625/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Very nice work!</pre>
<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/3625/diff/1/?file=59756#file59756line900" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed/main/core_unreal.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">struct ast_channel *ast_unreal_new_channels(struct ast_unreal_pvt *p,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">897</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_format</span> <span class="n">fmt</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">900</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_format</span> <span class="o"><span class="hl">*</span></span><span class="n">fmt</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;">Given the number of off nominal paths *and* the fact that this will be de-ref'd on the nominal path, you may want to consider using RAII_VAR here.
It has been used inappropriately in other places, but this one may make some sense.</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/3625/diff/1/?file=59756#file59756line935" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed/main/core_unreal.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">struct ast_channel *ast_unreal_new_channels(struct ast_unreal_pvt *p,</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">931</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="n">ao2_ref</span><span class="p">(</span><span class="n">p</span><span class="p">,</span> <span class="o">-</span><span class="mi">1</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">932</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="n">ast_channel_unlock</span><span class="p">(</span><span class="n">owner</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 is one of those weird-isms about core_unreal:
While its pvt is ao2 ref counted, the channel core does not expect a channel's pvt to be a ref counted object. Hence, if you destruct a channel and that channel still has a pvt, it will attempt to ast_free the pvt.
Since the channel was just allocated here, that means this will destroy the channel and cause a memory corruption. (Yup, there's a bunch of places where this happens in here - whoops)
In the off nominal paths, make sure you call ast_channel_tech_pvt_set(owner, NULL) - or move the association of the owner/pvt later on in the code.</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/3625/diff/1/?file=59757#file59757line3122" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed/main/data.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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 int manager_data_get(struct mansession *s, const struct message *m)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">3121</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="tb"> </span><span class="n">ast_data_add_int</span><span class="p">(</span><span class="n">codec</span><span class="p">,</span> <span class="s">"frame_length"</span><span class="p">,</span> <span class="n">fmlist</span><span class="p">[</span><span class="n">x</span><span class="p">].</span><span class="n">fr_len</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">3117</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ast_data_add_str</span><span class="p">(</span><span class="n">codec</span><span class="p">,</span> <span class="s">"description"</span><span class="p">,</span> <span class="n">tmp</span><span class="o">-></span><span class="n">description</span><span class="p">);</span></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">3118</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="cm">/* </span><span class="cs">BUG</span><span class="cm">: no more fr_len, I couldn't see the replacement */</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">3119</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="cm">/* ast_data_add_int(codec, "frame_length", tmp->fr_len); */</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">3120</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ao2_ref</span><span class="p">(</span><span class="n">tmp</span><span class="p">,</span> <span class="o">-</span><span class="mi">1</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">3121</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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;">Is/was fr_len used anywhere else?</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/3625/diff/1/?file=59761#file59761line340" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed/main/format_compatibility.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">int ast_format_compatibility_bitfield2cap(uint64_t bitfield, struct ast_format_cap *cap)</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">340</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c">#error </span><span class="cs">BUG</span><span class="c">: these procedures need to be converted</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;">As pedantic as it may seem, I'd convert BUG to BUGBUG.
"BUG" shows up too much for grep to be useful. BUGBUG has been rather helpful in finding things that need to get completed before the code is considered "finished"</pre>
</div>
<br />
<p>- Matt Jordan</p>
<br />
<p>On June 19th, 2014, 9:50 a.m. CDT, Corey Farrell 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.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Corey Farrell.</div>
<p style="color: grey;"><i>Updated June 19, 2014, 9:50 a.m.</i></p>
<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;">Updates to allow most of the Asterisk core to compile. I've excluded main/channel.c, main/dsp.c and main/rtp_engine.c. Changes to those files will be posted separate since I feel they are more complex and likely to have more error's. If any of the files included in this review fit that description let me know and I will split them off.
This change does not include any replacement for calls to ast_format_is_slinear(), and adds it back to the header (but does not implement). So ast_format_is_slinear hasn't been fixed, just deferred to become a link error.
The modifications to chan_phone are to allow what I believe to be a comparability function to be in the correct namespace to be implemented in format_compatibility.c.</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;">Compiled.</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/media_formats-reviewed/main/stasis_channels.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/sounds_index.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/sorcery.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/slinfactory.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/media_index.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/manager.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/indications.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/image.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/frame.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/format_pref.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/format_compatibility.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/format.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/file.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/dial.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/data.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/core_unreal.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/core_local.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/main/codec.c <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/include/asterisk/slinfactory.h <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/include/asterisk/rtp_engine.h <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/include/asterisk/format_pref.h <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/include/asterisk/format_compatibility.h <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/include/asterisk/format_cache.h <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/include/asterisk/format.h <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/include/asterisk/file.h <span style="color: grey">(416235)</span></li>
<li>/team/group/media_formats-reviewed/channels/chan_phone.c <span style="color: grey">(416235)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3625/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>