<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/1574/">https://reviewboard.asterisk.org/r/1574/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 8th, 2011, 7:56 p.m., <b>Simon Perreault</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/1574/diff/1/?file=21661#file21661line14417" 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 void truncate_hostport(char *hostport)</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">14417</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">c</span> <span class="o">=</span> <span class="n">strrchr</span><span class="p">(</span><span class="n">hostport</span><span class="p">,</span> <span class="sc">':'</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;">This will fail with IPv6 address literals.
e.g. "[2001:db8::1]:1234"
You need to check for brackets. (Unless I'm mistaken about this function's expected input range, in which case this should be documented in the doxygen comments.)
Isn't there code in netsock2.c that already does this?</pre>
</blockquote>
<p>On November 8th, 2011, 9:20 p.m., <b>Terry Wilson</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;">Actually, it would work fine on the example you gave because I am using 'strrchr' not 'strchr'. Where it would fail, though, would be if the function was used on an IPv6 address with no port. Brackets wouldn't make much difference either way. I will check to see if I use it somewhere where we might get such an address. I probably do.
There appears to be a get_domain() function in chan_sip.c, but it looks like it would treat the first unbracketed ':' as a port demarcation and set it to NULL as well, which would be bad. We might want to check and make sure it is never used on something that might be an IPv6 address as well.
I may be missing the netsock2 code you are talking about. I suppose we could parse the host:port into an ast_sockaddr with PARSE_PORT_IGNORE then print it back out, but that sounds like a lot of overhead. I can keep looking at it. If you run into the function you are thinking of, please let me know.</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;">Actually, I did miss the netsock function. ast_sockaddr_split_hostport(). It is so accurately named, I couldn't help but miss it. :-)</pre>
<br />
<p>- Terry</p>
<br />
<p>On November 8th, 2011, 7:41 p.m., Terry Wilson 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 Terry Wilson.</div>
<p style="color: grey;"><i>Updated Nov. 8, 2011, 7:41 p.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;">In reviewing https://reviewboard.asterisk.org/r/1533/ I noticed that going from 1.4 to 1.8, setting domain=x.x.x.x stopped matching when receiving a request to x.x.x.x. The reason is that the parse_uri function returns host:port in what was labeled the "domain" variable in 1.8 when in 1.4 the parsing was done in each individual function and the port truncated manually. For example, peer->fromdomain could be set to a host:port, even though it is specifically split into peer->fromdomain and peer->fromport other places in the code.
We can't change the parsing function because sometimes we actually use the host:port form of the result (as in when passing it to ast_sockaddr_resolve_first), so we need to do the truncation manually when appropriate. This patch does two things: 1) it renames the "domain" variables in the API functions to "hostport" to more accurately reflect what we are really returning and 2) implements a truncate_hostport function and uses it where appropriate.</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;">Ran the SIP unit tests and SIP-related testsuite tests and saw no difference in behavior. Set domain=127.0.0.1 and verified register requests started working.</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>/branches/1.8/channels/chan_sip.c <span style="color: grey">(343673)</span></li>
<li>/branches/1.8/channels/sip/include/reqresp_parser.h <span style="color: grey">(343673)</span></li>
<li>/branches/1.8/channels/sip/include/sip.h <span style="color: grey">(343673)</span></li>
<li>/branches/1.8/channels/sip/reqresp_parser.c <span style="color: grey">(343673)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1574/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>