<p>Matthew Fredrickson <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/9174">View Change</a></p><p>Patch set 3:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(14 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/dundi-parser.c">File pbx/dundi-parser.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/dundi-parser.c@588">Patch Set #3, Line 588:</a> <code style="font-family:monospace,monospace"> return dundi_ie_append_raw(ied, ie, sin, (int)sizeof(struct ast_sockaddr));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This looks like it might be a protocol breaking change due to struct sizes being potentially different and struct arrangement potentially being different. Can you confirm or deny this? (What happens if you connect systems w/o this change to systems with this change).</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c">File pbx/pbx_dundi.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@a4846">Patch Set #3, Line 4846:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why is hostname lookup and public IP address resolution being removed here?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@a4867">Patch Set #3, Line 4867:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why is this code being removed (this block of code) used for resolving ipaddr?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@a5013">Patch Set #3, Line 5013:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Where is sin.s_addr set to INADDR_ANY now?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@1708">Patch Set #3, Line 1708:</a> <code style="font-family:monospace,monospace"> snprintf(data,sizeof(data),"%s:%d",ast_sockaddr_stringify(&trans->addr),expire);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Good use of ast_sockaddr_stringify and friends! As a matter of code convention we typically expect spacing between the ',' and ast_sockaddr_stringify, as well as between ',' and expire. So like this:<br>snprintf(data,sizeof(data), "%s:%d", ast_sockaddr_stringify(&trans->addr), expire);</p><p style="white-space: pre-wrap; word-wrap: break-word;">There are a few other places in your patch where this needs addressed as well I think. I'll point them out as I catch them.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@1710">Patch Set #3, Line 1710:</a> <code style="font-family:monospace,monospace"> if ( ast_sockaddr_cmp(&peer->addr,&trans->addr)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Preceding space between the '(' and "ast_sockaddr_cmp" needs to be removed.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Function argument spacing here needs to be fixed (as mentioned above).</p><p style="white-space: pre-wrap; word-wrap: break-word;">So I'd change this line to:<br>if (ast_sockaddr_cmp(&peer->addr, &trans->addr))</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@2727">Patch Set #3, Line 2727:</a> <code style="font-family:monospace,monospace"> &peer->eid),ast_sockaddr_stringify_host(&peer->addr),</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Function argument spacing needs to be fixed here to:<br>&peer->eid), ast_sockaddr_stringify_host(&peer->addr),</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@4536">Patch Set #3, Line 4536:</a> <code style="font-family:monospace,monospace"> if ( ast_sockaddr_resolve(&addrs,data,PARSE_PORT_FORBID,AF_UNSPEC) > 0){</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to put spaces between arguments here.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@4537">Patch Set #3, Line 4537:</a> <code style="font-family:monospace,monospace"> ast_sockaddr_copy(&peer->addr,&addrs[0]);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Also here.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@4540">Patch Set #3, Line 4540:</a> <code style="font-family:monospace,monospace"> ast_sockaddr_set_port(&peer->addr,port);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">And here.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@4666">Patch Set #3, Line 4666:</a> <code style="font-family:monospace,monospace"> if ( ast_sockaddr_port(&peer->addr) == 0 ){</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Needs to be changed to:<br>if (ast_sockaddr_port(&peer->addr) == 0) {</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@4667">Patch Set #3, Line 4667:</a> <code style="font-family:monospace,monospace"> ast_sockaddr_set_port(&peer->addr,DUNDI_PORT);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Spaces between function arguments here need to be added.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@4912">Patch Set #3, Line 4912:</a> <code style="font-family:monospace,monospace"> if ( !ast_sockaddr_port(sin)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Change to:<br>if (!ast_sockaddr_port(sin)) {</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@4913">Patch Set #3, Line 4913:</a> <code style="font-family:monospace,monospace"> ast_sockaddr_set_port(sin,DUNDI_PORT);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Space needed:<br>ast_sockaddr_set_port(sin, DUNDI_PORT);</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/9174">change 9174</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/9174"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ia9e8dc3d153de7a291dbda4bd87fc827dd2bb846 </div>
<div style="display:none"> Gerrit-Change-Number: 9174 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Kirsty Tyerman <kirsty.tyerman@boeing.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kirsty Tyerman <kirsty.tyerman@boeing.com> </div>
<div style="display:none"> Gerrit-Reviewer: Matthew Fredrickson <creslin@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 15 Jun 2018 19:15:11 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>