<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/1341/">https://reviewboard.asterisk.org/r/1341/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 3rd, 2011, 10:40 a.m., <b>rmudgett</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;">Is there going to be a version for 1.8,...?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Right now, I'm thinking not. It was being categorized as a feature change in the issue comments.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 3rd, 2011, 10:40 a.m., <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/1341/diff/1/?file=17706#file17706line11870" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/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 void initreqprep(struct sip_request *req, struct sip_pvt *p, int sipmethod, const char * const explicit_uri)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">11864</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ourport</span> <span class="o">=</span> <span class="p">(</span><span class="n">p</span><span class="o">-></span><span class="n">fromdomainport</span><span class="p">)</span> <span class="o">?</span> <span class="n">p</span><span class="o">-></span><span class="n">fromdomainport</span> <span class="o">:</span> <span class="n">ast_sockaddr_port</span><span class="p">(</span><span class="o">&</span><span class="n">p</span><span class="o">-></span><span class="n">ourip</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">11870</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ourport</span> <span class="o">=</span> <span class="p">(</span><span class="n">p</span><span class="o">-></span><span class="n">fromdomainport</span><span class="p">)</span> <span class="o">?</span> <span class="n">p</span><span class="o">-></span><span class="n">fromdomainport</span> <span class="o">:</span> <span class="n">ast_sockaddr_port</span><span class="p">(</span><span class="o">&</span><span class="n">p</span><span class="o">-></span><span class="n">ourip</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">11871</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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">11872</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/* If a caller id name was specified, add a display name. */</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">11873</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">cid_has_name</span><span class="p">)</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">11874</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">snprintf</span><span class="p">(</span><span class="n">from</span><span class="p">,</span> <span class="k">sizeof</span><span class="p">(</span><span class="n">from</span><span class="p">),</span> <span class="s">"</span><span class="se">\"</span><span class="s">%s</span><span class="se">\"</span><span class="s"> "</span><span class="p">,</span> <span class="n">n</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">11875</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <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">11876</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">11865</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">sip_standard_port</span><span class="p">(</span><span class="n">p</span><span class="o">-></span><span class="n">socket</span><span class="p">.</span><span class="n">type</span><span class="p">,</span> <span class="n">ourport</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">11877</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">sip_standard_port</span><span class="p">(</span><span class="n">p</span><span class="o">-></span><span class="n">socket</span><span class="p">.</span><span class="n">type</span><span class="p">,</span> <span class="n">ourport</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;">Make code like:
ourport = ...
if (cid_has_name) {
/* Add a display name */
sprintf...
} else {
from[0] = '\0';
}
if (!sip_stand....
1) Code is grouped for the purpose of generating the from string.
2) The from[0] initialization is delayed until needed and you don't have to look for it.</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;">Alright, thanks, I was actually dubious over that to start with.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 3rd, 2011, 10:40 a.m., <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/1341/diff/1/?file=17706#file17706line11877" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/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 void initreqprep(struct sip_request *req, struct sip_pvt *p, int sipmethod, const char * const explicit_uri)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">11865</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">sip_standard_port</span><span class="p">(</span><span class="n">p</span><span class="o">-></span><span class="n">socket</span><span class="p">.</span><span class="n">type</span><span class="p">,</span> <span class="n">ourport</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">11877</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">sip_standard_port</span><span class="p">(</span><span class="n">p</span><span class="o">-></span><span class="n">socket</span><span class="p">.</span><span class="n">type</span><span class="p">,</span> <span class="n">ourport</span><span class="p">))</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">11866</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">snprintf</span><span class="p">(</span><span class="n">from</span><span class="p">,</span> <span class="k">sizeof</span><span class="p">(</span><span class="n">from</span><span class="p">),</span> <span class="s">"</span><span class="se"><span class="hl">\"</span></span><span class="s"><span class="hl">%s</span></span><span class="se"><span class="hl">\"</span></span><span class="s"><span class="hl"> </span><sip:%s@%s:%d>;tag=%s"</span><span class="p"><span class="hl">,</span></span><span class="hl"> </span><span class="n"><span class="hl">n</span></span><span class="p">,</span> <span class="n">l</span><span class="p">,</span> <span class="n">d</span><span class="p">,</span> <span class="n">ourport</span><span class="p">,</span> <span class="n">p</span><span class="o">-></span><span class="n">tag</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">11878</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">snprintf</span><span class="p">(</span><span class="o"><span class="hl">&</span></span><span class="n">from</span><span class="p"><span class="hl">[</span></span><span class="n"><span class="hl">strlen</span></span><span class="p"><span class="hl">(</span></span><span class="n"><span class="hl">from</span></span><span class="p"><span class="hl">)]</span>,</span> <span class="k">sizeof</span><span class="p">(</span><span class="n">from</span><span class="p">)</span><span class="hl"> </span><span class="o"><span class="hl">-</span></span><span class="hl"> </span><span class="n"><span class="hl">strlen</span></span><span class="p"><span class="hl">(</span></span><span class="n"><span class="hl">from</span></span><span class="p"><span class="hl">)</span>,</span> <span class="s">"<sip:%s@%s:%d>;tag=%s"</span><span class="p">,</span> <span class="n">l</span><span class="p">,</span> <span class="n">d</span><span class="p">,</span> <span class="n">ourport</span><span class="p">,</span> <span class="n">p</span><span class="o">-></span><span class="n">tag</span><span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">11867</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span> <span class="k">else</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">11879</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span> <span class="k">else</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">11868</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">snprintf</span><span class="p">(</span><span class="n">from</span><span class="p">,</span> <span class="k">sizeof</span><span class="p">(</span><span class="n">from</span><span class="p">),</span> <span class="s">"</span><span class="se"><span class="hl">\"</span></span><span class="s"><span class="hl">%s</span></span><span class="se"><span class="hl">\"</span></span><span class="s"><span class="hl"> </span><sip:%s@%s>;tag=%s"</span><span class="p">,</span> <span class="n"><span class="hl">n</span></span><span class="p"><span class="hl">,</span></span><span class="hl"> </span><span class="n">l</span><span class="p">,</span> <span class="n">d</span><span class="p">,</span> <span class="n">p</span><span class="o">-></span><span class="n">tag</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">11880</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">snprintf</span><span class="p">(</span><span class="o"><span class="hl">&</span></span><span class="n">from</span><span class="p"><span class="hl">[</span></span><span class="n"><span class="hl">strlen</span></span><span class="p"><span class="hl">(</span></span><span class="n"><span class="hl">from</span></span><span class="p"><span class="hl">)]</span>,</span> <span class="k">sizeof</span><span class="p">(</span><span class="n">from</span><span class="p">)</span><span class="hl"> </span><span class="o"><span class="hl">-</span></span><span class="hl"> </span><span class="n"><span class="hl">strlen</span></span><span class="p"><span class="hl">(</span></span><span class="n"><span class="hl">from</span></span><span class="p"><span class="hl">)</span>,</span> <span class="s">"<sip:%s@%s>;tag=%s"</span><span class="p">,</span> <span class="n">l</span><span class="p">,</span> <span class="n">d</span><span class="p">,</span> <span class="n">p</span><span class="o">-></span><span class="n">tag</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;">Remember that strlen() takes time to execute and you are calling it 4 times here. Just save the strlen() value before the if and use it in the 4 places.</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;">Well, there are 4 calls, but it will only be called twice in any execution of the function. Also keep in mind that only the display name can be entered into the from string at this point, so realistically we are talking like 30 characters on a bad day (and 0 characters in the case where we didn't have a caller id name) in a function that takes O(n) time based on length of the string that is going to be ran on a typical sip call once or twice at the beginning... I was thinking about it at the time, but it was at that level uncomfortably low impact that I almost thought it would be worse to have another variable floating around.
That said, I do hate running functions twice in one statement, so I'll go ahead and use temp variables, but I think it would be better to have them before each snprintf statement than before the condition since they shouldn't have function-wide scope.</pre>
<br />
<p>- jrose</p>
<br />
<p>On August 3rd, 2011, 9:57 a.m., jrose 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, David Vossel and rmudgett.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated Aug. 3, 2011, 9:57 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;">This is basically what I was trying to do with the user submitted patch from yesterday. After speaking with Richard, it became somewhat obvious that the approach to the patch had too much extra impact on the SIP messages when all that was really needed was specifically targeting the creation of the display name.
This approach checks strictly for a caller id name and if it isn't there, we skip the creation of the display name. I think this should be safer in general than the previous approach.</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;">Testing was similar to last time with focus on comparing the SIP messages between this branch and the normal trunk. In this case, only the display name gets changed.
A few SIP tests from the test suite were also ran successfully. I haven't ran the full test suite yet, but am preparing to do so.</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-16198">ASTERISK-16198</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>/trunk/CHANGES <span style="color: grey">(330571)</span></li>
<li>/trunk/channels/chan_sip.c <span style="color: grey">(330571)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1341/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>