<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/2298/">https://reviewboard.asterisk.org/r/2298/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Looks good to me. Minor note included - feel free to ignore.</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/2298/diff/1/?file=33124#file33124line10238" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/channels/chan_sip.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 process_sdp_a_image(const char *a, struct sip_pvt *p)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">10227</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span> <span class="k">else</span> <span class="k">if</span> <span class="p">((</span><span class="n">strncmp</span><span class="p">(</span><span class="n">a</span><span class="p">,</span> <span class="s">"<span class="hl">T38FaxF</span>ill<span class="hl">BitR</span>emoval"</span><span class="p">,</span> <span class="mi">20</span><span class="p">)</span> <span class="o">==</span> <span class="mi">0</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">10238</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span> <span class="k">else</span> <span class="k">if</span> <span class="p">((</span><span class="n">strncmp</span><span class="p">(</span><span class="n">a<span class="hl">ttrib</span></span><span class="p">,</span> <span class="s">"<span class="hl">t38faxf</span>ill<span class="hl">bitr</span>emoval"</span><span class="p">,</span> <span class="mi">20</span><span class="p">)</span> <span class="o">==</span> <span class="mi">0</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;">Unrelated to the overall intent of the patch, but I would update these strncmp calls to something like this:
strncmp(attrib, "foo", sizeof("foo") - 1)</pre>
</div>
<br />
<p>- Sean</p>
<br />
<p>On January 29th, 2013, 9:13 a.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 wdoekes.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated Jan. 29, 2013, 9:13 a.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;">RFC5347 section 2.5.2 states the following:
...
The attribute "T38MaxBitRate" was once incorrectly registered with
IANA as "T38maxBitRate" (lower-case "m"). In accordance with T.38
examples and common implementation practice, the form "T38MaxBitRate"
SHOULD be generated by implementations conforming to this package.
In general, it is RECOMMENDED that implementations of this package
accept lowercase, uppercase, and mixed upper/lowercase encodings of
all the T.38 attributes.
...
Asterisk currently does not perform case insensitive matching on the T.38 attributes. This causes the T38MaxBitRate attribute to be negotiated at 2400 baud instead of 14400 (or whatever value you actually wanted).
Given that we use sscanf, there's two ways to go about solving this:
* Convert the received attribute to lower/upper case and compare that way (this is what this patch does)
* Use sscanf to match both lower/upper case letters for every letter in the attribute, i.e., sscanf(a, "%*[tT]38%*[mM]%*[aA]%*[xX]%*[bB]%*[iI]%*[tT]%*[rR]%*[aA]%*[tT]%*[eE]:%30u", &x)
I went with the all lower option as it's a lot easier to read. The sscanf option is a bit more performant, as you don't have to convert the attribute to another case. (Note that as Walter suggested on ASTERISK-20897, we could write our own tolower method that did this a bit faster, but that feels like pre-mature optimization in this case).
If folks feel strongly about using a slightly faster approach, I'm open to that.</pre>
</td>
</tr>
</table>
<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-20897">ASTERISK-20897</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/channels/chan_sip.c <span style="color: grey">(380332)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2298/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>