<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/1278/">https://reviewboard.asterisk.org/r/1278/</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;">This patch is important and well executed. I especially like the new tests file.
There is one design issue I'm not too sure about: the need for sip_sanitized_host(). Would it be possible to use *_remote() when creating p->tohost and r->hostname instead? Is there any reason at all why we would want the scope id in there? The only reason I can see is if we use p->tohost or r->hostname to create a socket. In that case the patch is fine as it stands, otherwise it would be better to do away with sip_sanitized_host().</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/1278/diff/1/?file=17111#file17111line12653" 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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static const char *sip_sanitized_host(const char *host)</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">12653</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="o">!</span><span class="n">ast_sockaddr_parse</span><span class="p">(</span><span class="o">&</span><span class="n">addr</span><span class="p">,</span> <span class="n">host</span><span class="p">,</span> <span class="n">PARSE_PORT_FORBID</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;">Couldn't p->tohost contain a port number? If so, then PARSE_PORT_FORBID should be replaced with 0.
In fact, PARSE_PORT_FORBID should be used when there's a hard reason why a port is unacceptable. If there is one, please add it in comments.</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/1278/diff/1/?file=17113#file17113line105" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/netsock2.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; ">char *ast_sockaddr_stringify_fmt(const struct ast_sockaddr *sa, int format)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">98</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">switch</span> <span class="p">(</span><span class="n">format</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">105</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">switch</span> <span class="p">(</span><span class="n">format</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;">Here it would be better to leave the case labels as they were and instead do:
switch (format & (AST_SOCKADDR_STR_ADDR | AST_SOCKADDR_STR_PORT | AST_SOCKADDR_STR_BRACKETS)) {
Feel free to #define something like AST_SOCKADDR_STR_FORMAT_MASK instead of hard-coding the three bits as I did above.</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/1278/diff/1/?file=17113#file17113line408" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/netsock2.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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int ast_sockaddr_is_ipv6_link_local(const struct ast_sockaddr *addr)</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">408</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span> <span class="n">ast_sockaddr_is_ipv6</span><span class="p">(</span><span class="n">addr</span><span class="p">)</span> <span class="o">&&</span> <span class="n">addr</span><span class="o">-></span><span class="n">len</span> <span class="o">&&</span> <span class="n">IN6_IS_ADDR_LINKLOCAL</span><span class="p">(</span><span class="o">&</span><span class="n">sin6</span><span class="o">-></span><span class="n">sin6_addr</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;">The "&& addr->len" part is unnecessary because ast_sockaddr_is_ipv6() already checks it.</pre>
</div>
<br />
<p>- Simon</p>
<br />
<p>On June 22nd, 2011, 12:28 a.m., Terry Wilson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.orgrb/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.</div>
<div>By Terry Wilson.</div>
<p style="color: grey;"><i>Updated 2011-06-22 00:28:19</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;">chan_sip incorrectly would send IPv6 link-local addresses with the scope-id in SIP messages. This is usually an interface name on the local machine and means nothing to the remote host and should not be sent.
The original patch submitted on the issue just stripped the '%' and everything after it from the host before doing any kind of stringification. I was uncomfortable with making a global change to how these functions operated and instead added new _remote versions of the stringification functions and had chan_sip use them when the results would end up in a SIP message. The non-remote versions of the functions are still used in things like debug messages and anywhere we might be using the information to make an outbound connection since the scope-id is necessary information in those cases.
Since registrations were the focus of the bug report, I also noticed that the URI, From, and To of the registration also included the scope-id and didn't use the stringification functions, so I added sip_sanitized_host() which would take a host and if it happened to be a link-local ipv6 address, would "do the right thing".
I also added some netsock2 parsing tests.
I am not an IPv6 guru, so I would appreciate someone who is verifying this patch is a good idea.</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;">I tested registering two asterisk boxes to each other using link-local ipv6 addresses. They failed before the patch and succeeded after.</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-17711">ASTERISK-17711</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">(324299)</span></li>
<li>/branches/1.8/include/asterisk/netsock2.h <span style="color: grey">(324299)</span></li>
<li>/branches/1.8/main/netsock2.c <span style="color: grey">(324299)</span></li>
<li>/branches/1.8/tests/test_netsock2.c <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1278/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>