<p><a href="https://gerrit.asterisk.org/9858">View Change</a></p><p>2 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/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@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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Please refer my comments and explanation in the commit message. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">A friend is going to attempt name matching first before it attempts IP address matching.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The problem with matching IP addresses using the peer->addr is that it is the contact address of the peer.  It is not necessarily the source address of the transport.  If we believe the contact header during registration we are unlikely to ever match by IP and port.  We believe the contact header during registration if NAT is not configured on the peer and the incoming REGISTER message does not have rport.</p><p style="white-space: pre-wrap; word-wrap: break-word;">How about this to shrink the number of unmatching cases:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (!peer->host_dynamic) {<br>   return CMP_MATCH;<br>}<br>/* Conditions taken from parse_register_contact() */<br>if (peer2->transports & (AST_TRANSPORT_WS | AST_TRANSPORT_WSS)) {<br>   /* The contact address of websockets is always the transport source address and port */<br>   return 0;<br>}<br>if (ast_test_flag(&peer->flags[0], SIP_NAT_FORCE_RPORT)) {<br>   /* The contact address of NATed peers is always the transport source address and port */<br>   return 0;<br>}<br>/* Have to assume that we used the registered contact header */<br>return CMP_MATCH;</pre></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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">It's still in use in load_module() for the peers_by_ip container (which is used in various places). […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It is dead.  It is referenced but never called.  That function will only get called if something uses ao2_find() on that container.  Nothing uses ao2_find() on that container so it will never get called.</p><p style="white-space: pre-wrap; word-wrap: break-word;">It is confusing because it isn't clear how peer_ipcmp_cb() gets called nor is it clear how it could do two passes.  Two passes is an implicit assumption built into peer_ipcmp_cb_full().</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: Thu, 23 Aug 2018 18:54:41 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>