[Asterisk-code-review] pbx dundi: Added IPv6 support for dundi (asterisk[15])

[Matthew Fredrickson]

Matthew Fredrickson has posted comments on this change. ( https://gerrit.asterisk.org/9174 )

Change subject: pbx_dundi: Added IPv6 support for dundi
......................................................................


Patch Set 3: Code-Review-1

(14 comments)

https://gerrit.asterisk.org/#/c/9174/3/pbx/dundi-parser.c
File pbx/dundi-parser.c:

https://gerrit.asterisk.org/#/c/9174/3/pbx/dundi-parser.c@588
PS3, Line 588: 	return dundi_ie_append_raw(ied, ie, sin, (int)sizeof(struct ast_sockaddr));
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).


https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c
File pbx/pbx_dundi.c:

https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@a4846
PS3, Line 4846: 
Why is hostname lookup and public IP address resolution being removed here?


https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@a4867
PS3, Line 4867: 
Why is this code being removed (this block of code) used for resolving ipaddr?


https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@a5013
PS3, Line 5013: 
Where is sin.s_addr set to INADDR_ANY now?


https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@1708
PS3, Line 1708: 				snprintf(data,sizeof(data),"%s:%d",ast_sockaddr_stringify(&trans->addr),expire);
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:
snprintf(data,sizeof(data), "%s:%d", ast_sockaddr_stringify(&trans->addr), expire);

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.


https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@1710
PS3, Line 1710: 				if ( ast_sockaddr_cmp(&peer->addr,&trans->addr)) {
Preceding space between the '(' and "ast_sockaddr_cmp" needs to be removed.

Function argument spacing here needs to be fixed (as mentioned above).

So I'd change this line to:
if (ast_sockaddr_cmp(&peer->addr, &trans->addr))


https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@2727
PS3, Line 2727: 			&peer->eid),ast_sockaddr_stringify_host(&peer->addr),
Function argument spacing needs to be fixed here to:
&peer->eid), ast_sockaddr_stringify_host(&peer->addr),


https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@4536
PS3, Line 4536: 				if ( ast_sockaddr_resolve(&addrs,data,PARSE_PORT_FORBID,AF_UNSPEC) > 0){
Need to put spaces between arguments here.


https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@4537
PS3, Line 4537: 					ast_sockaddr_copy(&peer->addr,&addrs[0]);
Also here.


https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@4540
PS3, Line 4540: 				ast_sockaddr_set_port(&peer->addr,port);
And here.


https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@4666
PS3, Line 4666: 	if ( ast_sockaddr_port(&peer->addr) == 0 ){
Needs to be changed to:
if (ast_sockaddr_port(&peer->addr) == 0) {


https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@4667
PS3, Line 4667: 		ast_sockaddr_set_port(&peer->addr,DUNDI_PORT);
Spaces between function arguments here need to be added.


https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@4912
PS3, Line 4912: 	if ( !ast_sockaddr_port(sin)) {
Change to:
if (!ast_sockaddr_port(sin)) {


https://gerrit.asterisk.org/#/c/9174/3/pbx/pbx_dundi.c@4913
PS3, Line 4913: 		ast_sockaddr_set_port(sin,DUNDI_PORT);
Space needed:
ast_sockaddr_set_port(sin, DUNDI_PORT);



-- 
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: 3
Gerrit-Owner: Kirsty Tyerman <kirsty.tyerman at boeing.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kirsty Tyerman <kirsty.tyerman at boeing.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Comment-Date: Fri, 15 Jun 2018 19:15:11 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180615/eef0583f/attachment.html>