[Asterisk-code-review] pbx dundi: Added IPv6 support for dundi (asterisk[15])
Joshua Colp
asteriskteam at digium.com
Tue Jun 12 07:14:07 CDT 2018
Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/9174 )
Change subject: pbx_dundi: Added IPv6 support for dundi
......................................................................
Patch Set 2: Code-Review-1
(25 comments)
https://gerrit.asterisk.org/#/c/9174/2/pbx/dundi-parser.c
File pbx/dundi-parser.c:
https://gerrit.asterisk.org/#/c/9174/2/pbx/dundi-parser.c@40
PS2, Line 40: extern int netsock_ai_family;
This doesn't appear to be necessary.
https://gerrit.asterisk.org/#/c/9174/2/pbx/dundi-parser.c@588
PS2, Line 588: return dundi_ie_append_raw(ied, ie, sin, (int)sizeof(struct ast_sockaddr));
What happens when a remote side (that does not have this change) receives this?
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c
File pbx/pbx_dundi.c:
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@302
PS2, Line 302: struct ast_sockaddr addr; /*!< Address of DUNDi peer */
Added redness
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@353
PS2, Line 353: static int get_ai_family(const char *address)
: {
: struct addrinfo hint, *res = NULL;
: int ret;
:
: memset(&hint, '\0', sizeof hint);
:
: hint.ai_family = PF_UNSPEC;
: hint.ai_flags = AI_NUMERICHOST;
:
: ret = getaddrinfo(address, NULL, &hint, &res);
: if (ret) {
: ast_log(LOG_ERROR, "Invalid address %s \n", address);
: return -1;
: }
:
: ret = res->ai_family;
:
: freeaddrinfo(res);
:
: return ret;
: }
I don't think this is necessary.
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@438
PS2, Line 438: cmp = ast_sockaddr_cmp(&trans->addr, sin);
Why the use of int cmp here?
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@487
PS2, Line 487: memcpy(&trans.addr, sin, sizeof(struct ast_sockaddr));
This should use ast_sockaddr_copy
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@1019
PS2, Line 1019: } else if (trans->parent->dr[z].weight > ies->answers[x]->weight) {
: /* Update weight if appropriate */
: trans->parent->dr[z].weight = ies->answers[x]->weight;
: }
Why the removal of the ntohs here?
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@1317
PS2, Line 1317: if ( ast_sockaddr_isnull(&trans->addr))
Extra space at the start before ast_sockaddr_isnull
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@1789
PS2, Line 1789: } else if (trans->parent->dr[z].weight > ies.answers[x]->weight) {
: /* Update weight if appropriate */
: trans->parent->dr[z].weight = ies.answers[x]->weight;
Also here - why the ntohs removal?
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@1865
PS2, Line 1865: ast_copy_string(trans->parent->dei->ipaddr,ast_sockaddr_stringify_addr(&trans->addr),sizeof(trans->parent->dei->ipaddr));
Spaces shouldn't be removed.
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@2096
PS2, Line 2096: res = ast_recvfrom(netsocket,buf,sizeof(buf),0,&sin);
Spacing.
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@2723
PS2, Line 2723: if (registeredonly && ast_sockaddr_isnull(&peer->addr))
Redness
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4563
PS2, Line 4563: for (i=0; i < cnt; i++)
: {
: if ( i ==0 ) {
: memcpy(&peer->addr,&addrs[i],sizeof(struct ast_sockaddr));
: peer->addr.ss.ss_family = get_ai_family(data);
: }
: ast_free(&addrs[i]);
: }
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.
Afterwards the entire thing can be freed using ast_free(addrs);
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4601
PS2, Line 4601: peer->addr.ss.ss_family = family;
: ast_sockaddr_set_port(&peer->addr,DUNDI_PORT);
These will get set during resolving/parsing so they don't need to be set. They will be initialized to null.
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4628
PS2, Line 4628: for (i=0; i < cnt; i++) {
: if ( i ==0 ) {
: memcpy(&peer->addr,&addrs[i],sizeof(struct ast_sockaddr));
: peer->addr.ss.ss_family = get_ai_family(v->value);
: peer->dynamic = 0;
: }
: ast_free(&addrs[i]);
: }
Same here.
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4640
PS2, Line 4640: }
This has been moved over so it doesn't line up
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4868
PS2, Line 4868: int port=0;
The port doesn't need to be stored like this
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4877
PS2, Line 4877: sin->ss.ss_family = AF_INET; /*set default protocol as IPV4 */
This doesn't need to be done.
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4894
PS2, Line 4894: if( get_ai_family(v->value)== AF_INET6) {
: struct in6_addr *ip6;
: ip6 = &((struct sockaddr_in6*)&sin->ss)->sin6_addr;
: if ( inet_pton(AF_INET6,v->value, ip6)< 0 ) {
: ast_log(LOG_WARNING, "Invalid host/IP '%s'\n", v->value);
: }
: sin->ss.ss_family = AF_INET6;
: sin->len = sizeof(struct sockaddr_in6);
: } else {
: struct ast_hostent hent;
: struct hostent *hep;
: struct in_addr *ip4;
: hep = ast_gethostbyname(v->value, &hent);
: if (hep) {
: ip4 = &((struct sockaddr_in*)&sin->ss)->sin_addr;
: memcpy(ip4, hep->h_addr, sizeof(*ip4));
: sin->len = sizeof(struct sockaddr_in);
: } else
: ast_log(LOG_WARNING, "Invalid host/IP '%s'\n", v->value);
: }
This just needs to use ast_sockaddr_parse to set this. It takes care of everything.
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4969
PS2, Line 4969: if ( port == 0) {
ast_sockaddr_port can be used here
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@4993
PS2, Line 4993: build_peer(&testeid, ast_variable_browse(cfg, cat), &globalpcmodel,sin->ss.ss_family);
These should use the ast_sockaddr_is_ipv4 or is_ipv6.
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@5069
PS2, Line 5069: return AST_MODULE_LOAD_DECLINE;
This leaks memory now by not going to declined.
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@5071
PS2, Line 5071: memset(&sin,0,sizeof(struct ast_sockaddr));
This should use ast_sockaddr_setnull.
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@5075
PS2, Line 5075: return AST_MODULE_LOAD_DECLINE;
This also leaks memory now, as do each below.
https://gerrit.asterisk.org/#/c/9174/2/pbx/pbx_dundi.c@5077
PS2, Line 5077: if (sin.ss.ss_family == AF_INET6) {
: netsocket = socket(AF_INET6, SOCK_DGRAM, IPPROTO_IP);
: }
: else {
: netsocket = socket(AF_INET, SOCK_DGRAM, IPPROTO_IP);
: }
This should use ast_sockaddr_is_ipv4 or ast_sockaddr_is_ipv6.
The other alternative is to use the netsock2 API further, and use it for the socket as well.
--
To view, visit https://gerrit.asterisk.org/9174
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9e8dc3d153de7a291dbda4bd87fc827dd2bb846
Gerrit-Change-Number: 9174
Gerrit-PatchSet: 2
Gerrit-Owner: Kirsty Tyerman <kirsty.tyerman at boeing.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Comment-Date: Tue, 12 Jun 2018 12:14:07 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180612/d56fc1e0/attachment-0001.html>
More information about the asterisk-code-review
mailing list