<p>Patch set 5:<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>13 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9858/5//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/5//COMMIT_MSG@27">Patch Set #5, Line 27:</a> <code style="font-family:monospace,monospace">irrespecive</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">spell</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/5//COMMIT_MSG@31">Patch Set #5, Line 31:</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;">For everything else, match only if we have a host= that is<br>not dynamic set for that peer.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This sentence needs updating because of the comment about the dynamic host.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/5//COMMIT_MSG@54">Patch Set #5, Line 54:</a> <code style="font-family:monospace,monospace">Use cases:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You need to reorganize the use cases you are explaining. You jump around a lot where the reader can get confused about what you are talking about.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/5//COMMIT_MSG@63">Patch Set #5, Line 63:</a> <code style="font-family:monospace,monospace"> - will match peer4 if there registered port matches; else</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/there/the/</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/5//COMMIT_MSG@64">Patch Set #5, Line 64:</a> <code style="font-family:monospace,monospace"> - will match peer2 if if insecure=port is set for peer2.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/if if/if/</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/5//COMMIT_MSG@66">Patch Set #5, Line 66:</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;">For 1 and 2, would always match the first defined peer, possibly<br>resulting in peer not allowed to use transport during a later check.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think you are missing "previously" as the first word in the sentence. Otherwise, this paragraph makes no sense for the new behavior. It makes more sense when describing previous behavior.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/5//COMMIT_MSG@73">Patch Set #5, Line 73:</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;">Previously WS and WSS peers would need to be dynamic to match correctly,<br>or only have one registration from a given source IP.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is confusing as you have not dealt much with WS/WSS and you don't have a use case defined for those transports.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9858/5/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/5/channels/chan_sip.c@34437">Patch Set #5, Line 34437:</a> <code style="font-family:monospace,monospace"> /* Now only return a match if the port matches, as well. */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/Now/On the first pass/</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/5/channels/chan_sip.c@34442">Patch Set #5, Line 34442:</a> <code style="font-family:monospace,monospace"> /* We can only reach here is peer2 is for SIP_INSECURE_PORT, in</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/here is/here if/</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/5/channels/chan_sip.c@34447">Patch Set #5, Line 34447:</a> <code style="font-family:monospace,monospace"> * to check the source IP, but only if the host isn't dynamic. This isn't</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This comment paragraph needs updating. See my comment below when checking the host_dynamic flags.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/5/channels/chan_sip.c@34456">Patch Set #5, Line 34456:</a> <code style="font-family:monospace,monospace"> } else</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The current code does not need the else keyword here because the if clause returns.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9858/5/channels/chan_sip.c@34457">Patch Set #5, Line 34457:</a> <code style="font-family:monospace,monospace"> return peer2->host_dynamic ? 0 : (CMP_MATCH | CMP_STOP);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The best you can do for the second pass that matches by IP address only is to just return "match" here for non-udp protocols.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The port of a static peer is going to be the default port or an explicit port to contact the peer. It is not likely to be the source port of the transport. For dynamic peers that require registration, the port won't always be the transport's source port but what the REGISTER's contact header says. (NAT not enabled or rport not set in the REGISTER message) Again, the received contact port is not likely to be the transport's source port.</p><ul><li>The indention is not needed here because the else keyword above can be removed.</li><li>FYI: peer2->host_dynamic is not set by the caller. I think you meant peer->host_dynamic. However, you should not use that flag for the reasons already stated.</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/5/channels/chan_sip.c@34460">Patch Set #5, Line 34460:</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;">This is dead code and needs to be removed. It can cause misinterpretation of how peer_ipcmp_cb_full() actually works because this function can never get called.</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: 5 </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 00:15:39 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>