<p>Joshua Colp <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/9174">View Change</a></p><p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(25 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/9174/2/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/2/pbx/dundi-parser.c@40">Patch Set #2, Line 40:</a> <code style="font-family:monospace,monospace">extern int netsock_ai_family;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This doesn't appear to be necessary.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/dundi-parser.c@588">Patch Set #2, 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;">What happens when a remote side (that does not have this change) receives this?</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/9174/2/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/2/pbx/pbx_dundi.c@302">Patch Set #2, Line 302:</a> <code style="font-family:monospace,monospace"> struct ast_sockaddr addr; /*!< Address of DUNDi peer */ </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Added redness</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@353">Patch Set #2, Line 353:</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 get_ai_family(const char *address)<br>{<br> struct addrinfo hint, *res = NULL;<br> int ret;<br><br> memset(&hint, '\0', sizeof hint);<br><br> hint.ai_family = PF_UNSPEC;<br> hint.ai_flags = AI_NUMERICHOST;<br><br> ret = getaddrinfo(address, NULL, &hint, &res);<br> if (ret) {<br> ast_log(LOG_ERROR, "Invalid address %s \n", address);<br> return -1;<br> }<br><br> ret = res->ai_family;<br><br> freeaddrinfo(res);<br><br> return ret;<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think this is necessary.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@438">Patch Set #2, Line 438:</a> <code style="font-family:monospace,monospace"> cmp = ast_sockaddr_cmp(&trans->addr, sin);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why the use of int cmp here?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@487">Patch Set #2, Line 487:</a> <code style="font-family:monospace,monospace"> memcpy(&trans.addr, sin, sizeof(struct ast_sockaddr));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should use ast_sockaddr_copy</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@1019">Patch Set #2, Line 1019:</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;"> } else if (trans->parent->dr[z].weight > ies->answers[x]->weight) {<br> /* Update weight if appropriate */<br> trans->parent->dr[z].weight = ies->answers[x]->weight;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why the removal of the ntohs here?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@1317">Patch Set #2, Line 1317:</a> <code style="font-family:monospace,monospace"> if ( ast_sockaddr_isnull(&trans->addr))</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Extra space at the start before ast_sockaddr_isnull</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@1789">Patch Set #2, Line 1789:</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;"> } else if (trans->parent->dr[z].weight > ies.answers[x]->weight) {<br> /* Update weight if appropriate */<br> trans->parent->dr[z].weight = ies.answers[x]->weight;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Also here - why the ntohs removal?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@1865">Patch Set #2, Line 1865:</a> <code style="font-family:monospace,monospace"> ast_copy_string(trans->parent->dei->ipaddr,ast_sockaddr_stringify_addr(&trans->addr),sizeof(trans->parent->dei->ipaddr));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Spaces shouldn't be removed.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@2096">Patch Set #2, Line 2096:</a> <code style="font-family:monospace,monospace"> res = ast_recvfrom(netsocket,buf,sizeof(buf),0,&sin);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Spacing.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@2723">Patch Set #2, Line 2723:</a> <code style="font-family:monospace,monospace"> if (registeredonly && ast_sockaddr_isnull(&peer->addr)) </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Redness</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4563">Patch Set #2, Line 4563:</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 (i=0; i < cnt; i++)<br> {<br> if ( i ==0 ) {<br> memcpy(&peer->addr,&addrs[i],sizeof(struct ast_sockaddr));<br> peer->addr.ss.ss_family = get_ai_family(data);<br> }<br> ast_free(&addrs[i]);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">A for loop is not necessary here. If the count is greater than 0 than the first resolved address can be copied using ast_sockaddr_copy to the peer address.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Afterwards the entire thing can be freed using ast_free(addrs);</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4601">Patch Set #2, Line 4601:</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;"> peer->addr.ss.ss_family = family;<br> ast_sockaddr_set_port(&peer->addr,DUNDI_PORT);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">These will get set during resolving/parsing so they don't need to be set. They will be initialized to null.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4628">Patch Set #2, Line 4628:</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 (i=0; i < cnt; i++) {<br> if ( i ==0 ) {<br> memcpy(&peer->addr,&addrs[i],sizeof(struct ast_sockaddr));<br> peer->addr.ss.ss_family = get_ai_family(v->value);<br> peer->dynamic = 0;<br> }<br> ast_free(&addrs[i]);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4640">Patch Set #2, Line 4640:</a> <code style="font-family:monospace,monospace"> }</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This has been moved over so it doesn't line up</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4868">Patch Set #2, Line 4868:</a> <code style="font-family:monospace,monospace"> int port=0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The port doesn't need to be stored like this</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4877">Patch Set #2, Line 4877:</a> <code style="font-family:monospace,monospace"> sin->ss.ss_family = AF_INET; /*set default protocol as IPV4 */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This doesn't need to be done.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4894">Patch Set #2, Line 4894:</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;"> if( get_ai_family(v->value)== AF_INET6) {<br> struct in6_addr *ip6;<br> ip6 = &((struct sockaddr_in6*)&sin->ss)->sin6_addr;<br> if ( inet_pton(AF_INET6,v->value, ip6)< 0 ) {<br> ast_log(LOG_WARNING, "Invalid host/IP '%s'\n", v->value);<br> }<br> sin->ss.ss_family = AF_INET6;<br> sin->len = sizeof(struct sockaddr_in6);<br> } else {<br> struct ast_hostent hent;<br> struct hostent *hep;<br> struct in_addr *ip4;<br> hep = ast_gethostbyname(v->value, &hent);<br> if (hep) {<br> ip4 = &((struct sockaddr_in*)&sin->ss)->sin_addr;<br> memcpy(ip4, hep->h_addr, sizeof(*ip4));<br> sin->len = sizeof(struct sockaddr_in);<br> } else<br> ast_log(LOG_WARNING, "Invalid host/IP '%s'\n", v->value);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This just needs to use ast_sockaddr_parse to set this. It takes care of everything.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4969">Patch Set #2, Line 4969:</a> <code style="font-family:monospace,monospace"> if ( port == 0) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_sockaddr_port can be used here</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4993">Patch Set #2, Line 4993:</a> <code style="font-family:monospace,monospace"> build_peer(&testeid, ast_variable_browse(cfg, cat), &globalpcmodel,sin->ss.ss_family);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">These should use the ast_sockaddr_is_ipv4 or is_ipv6.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@5069">Patch Set #2, Line 5069:</a> <code style="font-family:monospace,monospace"> return AST_MODULE_LOAD_DECLINE;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This leaks memory now by not going to declined.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@5071">Patch Set #2, Line 5071:</a> <code style="font-family:monospace,monospace"> memset(&sin,0,sizeof(struct ast_sockaddr));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should use ast_sockaddr_setnull.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@5075">Patch Set #2, Line 5075:</a> <code style="font-family:monospace,monospace"> return AST_MODULE_LOAD_DECLINE;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This also leaks memory now, as do each below.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@5077">Patch Set #2, Line 5077:</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;"> if (sin.ss.ss_family == AF_INET6) {<br> netsocket = socket(AF_INET6, SOCK_DGRAM, IPPROTO_IP);<br> }<br> else {<br> netsocket = socket(AF_INET, SOCK_DGRAM, IPPROTO_IP);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should use ast_sockaddr_is_ipv4 or ast_sockaddr_is_ipv6.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The other alternative is to use the netsock2 API further, and use it for the socket as well.</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: 2 </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-Comment-Date: Tue, 12 Jun 2018 12:14:07 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>