<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/3734/">https://reviewboard.asterisk.org/r/3734/</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 9th, 2014, 9:45 p.m. CDT, <b>Corey Farrell</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/3734/diff/2/?file=62646#file62646line324" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed-trunk/main/bridge_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; ">struct ast_bridge_channel *ast_bridge_channel_peer(struct ast_bridge_channel *bridge_channel)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">324</font></th>
<td bgcolor="#fdfebc" 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">ast_format_cmp</span><span class="p">(</span><span class="n">ast_channel_readformat</span><span class="p">(</span><span class="n">bridge_channel</span><span class="o">-></span><span class="n">chan</span><span class="p">),</span> <span class="n">bridge_channel</span><span class="o">-></span><span class="n">read_format</span><span class="p">)</span> <span class="o">==</span> <span class="n">AST_FORMAT_CMP_NOT_EQUAL</span><span class="p">)</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">324</font></th>
<td bgcolor="#fdfebc" 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"><span class="hl">bridge_channel</span></span><span class="o"><span class="hl">-></span></span><span class="n"><span class="hl">read_format</span></span><span class="hl"> </span><span class="o"><span class="hl">&&</span></span><span class="hl"> </span><span class="n">ast_format_cmp</span><span class="p">(</span><span class="n">ast_channel_readformat</span><span class="p">(</span><span class="n">bridge_channel</span><span class="o">-></span><span class="n">chan</span><span class="p">),</span> <span class="n">bridge_channel</span><span class="o">-></span><span class="n">read_format</span><span class="p">)</span> <span class="o">==</span> <span class="n">AST_FORMAT_CMP_NOT_EQUAL</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;">Check still needed or can we just assert?</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 one is a bit interesting, in that the formats on the channel get saved off, an attempt to make the channel compatible with everything else occurs - where new formats are placed on the channel - leading to this path getting hit.
I have mixed feelings about this: there's a lot going on leading up to this point, and I'm not sure we can always predict that there will never be a case where a format won't be NULL. We can try asserts and restore these checks if needed.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 9th, 2014, 9:45 p.m. CDT, <b>Corey Farrell</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/3734/diff/2/?file=62649#file62649line118" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed-trunk/main/codec_builtin.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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">118</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">.</span><span class="n">type</span> <span class="o">=</span> <span class="n">AST_MEDIA_TYPE_AUDIO</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;">I'm not sure AUDIO is appropriate. Maybe UNKNOWN is better, or even a new AST_MEDIA_TYPE_NULL?</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;">If this isn't audio, we're going to get more NULL format hits.
A lot of things assume that there is a single audio format on the channel; consider every single format that is passed to the translation core. Whether or not that's appropriate is another question; but if the point of this is to try and prevent seg faults from a channel with no formats getting abused, not using audio is going to lead to crashing.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 9th, 2014, 9:45 p.m. CDT, <b>Corey Farrell</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/3734/diff/2/?file=62650#file62650line437" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed-trunk/main/format_cache.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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int ast_format_cache_set(const char *name, struct ast_format *format)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">429</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">int</span> <span class="nf">ast_format_cache_set</span><span class="p">(</span><span class="k"><span class="hl">const</span></span><span class="hl"> </span><span class="kt"><span class="hl">char</span></span><span class="hl"> </span><span class="o"><span class="hl">*</span></span><span class="n"><span class="hl">name</span></span><span class="p"><span class="hl">,</span></span><span class="hl"> </span><span class="k">struct</span> <span class="n">ast_format</span> <span class="o">*</span><span class="n">format</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">437</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">int</span> <span class="nf">ast_format_cache_set</span><span class="p">(</span><span class="k">struct</span> <span class="n">ast_format</span> <span class="o">*</span><span class="n">format</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 procedure could previously unset a format:
ast_format_cache_set("ulaw", NULL);
Is that not needed?
Also ast_format_get_name is called many times, it would be better to call it once and save to char *name;</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;">It isn't needed.
The name field is actually broken: you can set a format by some random name, but that name is not cached with a format; hence, you cannot look up a format using the name you provided.
Instead, you need to use ast_format_create_named. That will allow you to assign a custom name to format, which is equivalent to what this used to do.
Caching the result of ast_format_get_name is unlikely to be useful; compilers tend to handle simple accessors just fine.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 9th, 2014, 9:45 p.m. CDT, <b>Corey Farrell</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/3734/diff/2/?file=62650#file62650line442" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/media_formats-reviewed-trunk/main/format_cache.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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int ast_format_cache_set(const char *name, struct ast_format *format)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">434</font></th>
<td bgcolor="#fdfebc" 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">ast_strlen_zero</span><span class="p">(</span><span class="n">name</span><span class="p">))</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">442</font></th>
<td bgcolor="#fdfebc" 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="o"><span class="hl">!</span></span><span class="n"><span class="hl">format</span></span><span class="hl"> </span><span class="o"><span class="hl">||</span></span><span class="hl"> </span><span class="n">ast_strlen_zero</span><span class="p">(</span><span class="n"><span class="hl">ast_format_get_</span>name</span><span class="p"><span class="hl">(</span></span><span class="n"><span class="hl">format</span></span><span class="p"><span class="hl">)</span>))</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;">I think just assert for !format.</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;">No, this safety check is useful. It prevents someone from creating a named empty format and attempting to cache it.
Making an API robust - particularly when those checks do not sit in a critical path - is not a bad thing :-)</pre>
<br />
<p>- Matt</p>
<br />
<p>On July 9th, 2014, 7:37 p.m. CDT, Matt Jordan 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 Matt Jordan.</div>
<p style="color: grey;"><i>Updated July 9, 2014, 7:37 p.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;">This patch does two things:
* It updates a few of the unit tests for some of the API changes. In particular, it focuses on adding some tests for formats with attributes and their expected behaviour. A few other non-format related unit tests were updated as well to handle off nominals detected during testing.
* It adds an 'ast_format_none' format. This format is a dummy format that can be used instead of a NULL pointer to prevent having to put NULL dereference checks into every place in the codebase. Channels are no assigned this format immediately upon creation, and their default capabilities are set to have it. As this format's codec has no translation (nor a representation in the RTP engine), it _shouldn't_ cause harm.
* A few NULL checks were put in anyway into key areas in a few modules. These were ones that were hit hard by the unit tests and prone to crashing if presented a NULL format.</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;">Unit tests pass.
There is a FRACK on shutdown, but it doesn't appear to be caused by this patch (things didn't run long enough without this patch before)</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-trunk/tests/test_format_cap.c <span style="color: grey">(418259)</span></li>
<li>/team/group/media_formats-reviewed-trunk/tests/test_format_cache.c <span style="color: grey">(418259)</span></li>
<li>/team/group/media_formats-reviewed-trunk/tests/test_core_format.c <span style="color: grey">(418259)</span></li>
<li>/team/group/media_formats-reviewed-trunk/tests/test_cel.c <span style="color: grey">(418259)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/format_cap.c <span style="color: grey">(418259)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/format_cache.c <span style="color: grey">(418259)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/codec_builtin.c <span style="color: grey">(418259)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/codec.c <span style="color: grey">(418259)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/channel.c <span style="color: grey">(418259)</span></li>
<li>/team/group/media_formats-reviewed-trunk/main/bridge_channel.c <span style="color: grey">(418259)</span></li>
<li>/team/group/media_formats-reviewed-trunk/include/asterisk/format_cache.h <span style="color: grey">(418259)</span></li>
<li>/team/group/media_formats-reviewed-trunk/include/asterisk/codec.h <span style="color: grey">(418259)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3734/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>