<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/2400/">https://reviewboard.asterisk.org/r/2400/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 18th, 2013, 12:44 p.m., <b>elguero</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;">These were just some ideas that I had when looking at this review.</pre>
</blockquote>
<p>On March 18th, 2013, 1:57 p.m., <b>wdoekes</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;">Thanks for the review.
PARSE_PORT_REQUIRED is not good, since not supplying a port is valid.
Using ast_parse_arg() could be an option, I didn't know that existed.
Checking peer ports against >= 1024 is wrong. There is nothing that says a port cannot be below that. (And technically I think port 0 might be legal too.. but using that would be.. strange.)</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;">Ah, you are right... sorry for focusing on the port part of this code and also for temporarily crossing in my head client (peer) vs server on the port range part of this.</pre>
<br />
<p>- elguero</p>
<br />
<p>On March 18th, 2013, 9:16 a.m., wdoekes 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.</div>
<div>By wdoekes.</div>
<p style="color: grey;"><i>Updated March 18, 2013, 9:16 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;">The sent-by-addr address in the Via is supposed to get ignored and the sent-by-port is only used if we're not doing force_rport.
Nevertheless did check_via do hostname lookups even though it only wanted to get at the port.
I've changed the code around so it uses ast_sockaddr_split_hostport() instead of ast_sockaddr_resolve_first(). This also removes an ERROR notice which isn't worthy of such severity.
Before, when someone sent something resolvable in the Via:
chan_sip did an DNS A lookup, but ignored the results.
Now:
chan_sip just looks at the port.
Before, when someone sent a bad via header:
ERROR[29769]: netsock2.c:269 ast_sockaddr_resolve: getaddrinfo("127.0.1.1", "5061branch=z9hG4bK-11063-1-0", ...): Servname not supported for ai_socktype
WARNING[29769]: chan_sip.c:16921 check_via: Could not resolve socket address for '127.0.1.1:5061branch=z9hG4bK-11063-1-0'
Now:
WARNING[10229]: chan_sip.c:16928 check_via: Could not split host/port for '127.0.1.1:5061branch=z9hG4bK-11049-1-0'
</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;">57 SIP tests ran successfully with and without the patch.</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">(383308)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2400/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>