<p>George Joseph <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/7430">View Change</a></p><p>Patch set 1:</p><p>(1 comment)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/7430/1/channels/chan_sip.c">File channels/chan_sip.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7430/1/channels/chan_sip.c@34132">Patch Set #1, Line 34132:</a> <code style="font-family:monospace,monospace">    if (((peer->transports & peer2->transports) &</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">For TCP/TLS, the same connection must be re-used. I see no benefit<br>in insecure=port for TCP/TLS.</p><p style="white-space: pre-wrap; word-wrap: break-word;">However, David wrote something like "[For TCP,] the received<br>packet's destination port will not match the one the peer table is<br>built with." No idea what he was about. It is a pity that we cannot<br>ask David anymore.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The code before this change looks like insecure=port is enabled for<br>TCP/TLS always, regardless the settings in sip.conf. However, that<br>might have been not a TCP/TLS issue in general but a specific issue<br>at that time within Asterisk.</p><p style="white-space: pre-wrap; word-wrap: break-word;">By the way, did you see my previous reply in master? When peer2<br>has, and peer1 does not have insecure=port, zero is returned.<br>Instead, it should go through the port test and return zero only<br>when the ports do not match. Or? With other words, only when UDP<br>and both peers have insecure=port, then a match should be returned.<br>Otherwise the ports should be checked.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Ah, I didn't see the comment, sorry.  I think part of the problem is that peer_ipcmp_cb_full gets used in multiple ways.  It's used as the compare function for the peers_by_ip container when you call ao2_find on it.  In that case, there are 2 peers to compare.  It's also used by sip_find_peer when you only know the ip address or name.  In that case, peer2 is just a temporary peer created for the search.  I'm concerned that changing the default behavior of peer_ipcmp_cb_full would cause other problems because of teh multiple ways it's used.  Is there not something in the allowguest logic that could be tweaked instead?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/7430">change 7430</a>. To unsubscribe, 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/7430"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Id190428bf1d931f2dbfd4b293f53ff8f20d98efa </div>
<div style="display:none"> Gerrit-Change-Number: 7430 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Alexander Traud <pabstraud@compuserve.com> </div>
<div style="display:none"> Gerrit-Reviewer: Alexander Traud <pabstraud@compuserve.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 06 Dec 2017 14:15:02 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>