<p>Patch set 6:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/9858">View Change</a></p><p>22 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@26">Patch Set #6, Line 26:</a> <code style="font-family:monospace,monospace">It should be noted that when looking for a peer, two passes is made, the</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/is/are/</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@33">Patch Set #6, Line 33:</a> <code style="font-family:monospace,monospace">In previous behaviour the there was special handling for</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/the there/there/</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@61">Patch Set #6, Line 61:</a> <code style="font-family:monospace,monospace">4. If doing TCP, match if matched against peer also has</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/TCP/UDP/</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@95">Patch Set #6, Line 95:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * peer5 if registered from 1.1.1.1 and source port matches;<br> * peer1 or peer2 if from port 5060 (ordering); else<br> * peer3.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">pass 1:</p><ul><li>peer1 or peer2 if from port 5060</li><li>peer3 if from port 5061</li><li>peer5 if registered as 1.1.1.1 and source port matches</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">pass 2:</p><ul><li>peer3</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@98">Patch Set #6, Line 98:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> - current:<br> * peer5 if registered from 1.1.1.1 and source port matches;<br> * peer1 or peer2 if from port 5060 (ordering); else<br> * peer 3.<br></pre></blockquote></p><ul><li>current: as per previous</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@103">Patch Set #6, Line 103:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * peer5 if registered from 1.1.1.1 and source port matches;<br> * peer2 if from port 5060; else<br> * peer 3.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">pass 1:</p><ul><li>peer2 if port 5060</li><li>peer3 if port 5061</li><li>peer5 if registered as 1.1.1.1 and source port matches</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">pass 2:</p><ul><li>peer3</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@109">Patch Set #6, Line 109:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * peer5 if registered from 1.1.1.2 and port matches (irrespective of<br> transport protocol);<br> * peer4 if source port is 5060; else<br> * no match (guest).<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">pass 1:</p><ul><li>peer 4 if from port 5060</li><li>peer 5 if registered as 1.1.1.2 and source port matches</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">pass 2:</p><ul><li>no match (guest)</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@114">Patch Set #6, Line 114:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> - new:<br> * peer5 if registered from 1.1.1.2 and port matches (and udp<br> transport is allowed by the matched peer);<br> * peer4 if source port is 5060; else<br> * no match (guest).<br></pre></blockquote></p><ul><li>new: as per previous</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@122">Patch Set #6, Line 122:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * peer5 if registered from that address (and port if not insecure=port).<br> * no match (guest)<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">pass 1:</p><ul><li>peer 5 if registered as that address and source port matches</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">pass 2:</p><ul><li>no match (guest)</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@131">Patch Set #6, Line 131:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * peer1 or peer5 if peer5 registered from 1.1.1.1;<br> * peer1 (unless the source port happens to be 5060 then there is a<br> possibility based on ordering of matching peer2); else<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">pass 1:</p><ul><li>peer1</li><li>peer2 if from 5060</li><li>peer3 if from 5061</li><li>peer5 if registered as 1.1.1.1</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">pass 2:</p><ul><li>cannot happen as we would always find peer1 in the first pass</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@135">Patch Set #6, Line 135:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * peer5 if registered from 1.1.1.1 and source port matches;<br> * peer1 or peer2 if and only if source port happens to be 5060<br> (ordering);<br> * no match (guest)<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">pass 1:</p><ul><li>peer1 or peer2 if from 5060</li><li>peer3 if from 5061</li><li>peer5 if registered as 1.1.1.1 and source port matches</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">pass 2:</p><ul><li>no match (guest)</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@140">Patch Set #6, Line 140:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * peer5 if registered from 1.1.1.1 and source port matches;<br> * peer1.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">pass 1:</p><ul><li>peer1 if from 5060</li><li>peer5 if registered as 1.1.1.1 and source port matches</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">pass2:</p><ul><li>peer1</li><li>peer5 if registered as 1.1.1.1 <- We cannot eliminate dynamic peers because the registered CONTACT port is not guaranteed to be the source port of the message</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@145">Patch Set #6, Line 145:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * peer4 or peer5 if peer5 registered from 1.1.1.2<br> * peer4.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">pass 1:</p><ul><li>peer4</li><li>peer5 if registered as 1.1.1.2</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">pass 2:</p><ul><li>cannot happen as we would always find peer4 in the first pass</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@148">Patch Set #6, Line 148:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * peer5 if peer5 registered from 1.1.1.2 and source port matches;<br> * peer4 if source port happens to be port 5060; else<br> * no match (guest).<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">pass 1:</p><ul><li>peer4 if from 5060</li><li>peer5 if registered as 1.1.1.2 and source port matches</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">pass2:</p><ul><li>no match (guest)</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@152">Patch Set #6, Line 152:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * peer5 if peer5 registered from 1.1.1.2 and source port matches;<br> * peer4.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">pass 1:</p><ul><li>peer4 if from 5060</li><li>peer5 if registered as 1.1.1.2 and source port matches</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">pass2:</p><ul><li>peer4</li><li>peer5 if registered as 1.1.1.2 <- We cannot eliminate dynamic peers because the registered CONTACT port is not guaranteed to be the source port of the message</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@157">Patch Set #6, Line 157:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * peer5 if registered from that address; else<br> * no match (guest)<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">pass 1:</p><ul><li>peer 5 if registered as that address</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">pass 2:</p><ul><li>no match (guest)</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@160">Patch Set #6, Line 160:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * peer5 if registered from that address + port; else<br> * no match (guest)<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">pass 1:</p><ul><li>peer 5 if registered as that address and port</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">pass 2:</p><ul><li>no match (guest)</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@163">Patch Set #6, Line 163:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> * peer5 if registered from that address + port; else<br> * no match (guest)<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">pass 1:</p><ul><li>peer 5 if registered as that address and port</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">pass 2:</p><ul><li>peer 5 if registered as that address <- We cannot elimitate dynamic peers because the registered CONTACT port is not guaranteed to be the source port of the message</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@166">Patch Set #6, Line 166:</a> <code style="font-family:monospace,monospace">It should be noted the test cases doesn't make explicit mention of TLS,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/doesn't/don't/</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c">File channels/chan_sip.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c@34446">Patch Set #6, Line 34446:</a> <code style="font-family:monospace,monospace"> * for non-UDP the source port won't typically be controlled, we only want</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"can't" was a better word before where you have "won't" now.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c@34458">Patch Set #6, Line 34458:</a> <code style="font-family:monospace,monospace"> return peer->host_dynamic ? 0 : (CMP_MATCH | CMP_STOP);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">For the reasons I stated previously you need to just return CMP_MATCH here. You also need to update the comments and commit message to not consider dynamic vs static peers.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The peer->addr is the CONTACT address and is not necessarily the source address of the incoming message (especially the port). This is true for static peers and can be true for dynamic peers.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c@34461">Patch Set #6, Line 34461:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">static int peer_ipcmp_cb(void *obj, void *arg, int flags)<br>{<br> return peer_ipcmp_cb_full(obj, arg, NULL, flags);<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Delete this function for the reasons I stated previously.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/9858">change 9858</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/9858"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I61a9804e4feba9c7224c481f7a10bf7eb7c7f2a2 </div>
<div style="display:none"> Gerrit-Change-Number: 9858 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: Jaco Kroon <jaco@uls.co.za> </div>
<div style="display:none"> Gerrit-Reviewer: Jaco Kroon <jaco@uls.co.za> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 21 Aug 2018 20:14:17 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>