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

[Richard Mudgett]

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

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


Patch Set 6: Code-Review-1

(9 comments)

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

https://gerrit.asterisk.org/#/c/9174/6/pbx/pbx_dundi.c@a4849
PS6, Line 4849: 
              : 
              : 
Why was this check removed?


https://gerrit.asterisk.org/#/c/9174/6/pbx/pbx_dundi.c@2727
PS6, Line 2727: 			&peer->eid), ast_sockaddr_stringify_host(&peer->addr),
This is a change to the command behavior.  If peer->addr is not set or uninitialized then we output "(null)" now instead of "(Unspecified)".

If we were to include or exclude peers with an unspecified address we won't get what we would get before.


https://gerrit.asterisk.org/#/c/9174/6/pbx/pbx_dundi.c@2744
PS6, Line 2744: 					ast_sockaddr_stringify_host(&peer->addr),
This is a change to the command behavior.  If peer->addr is not set or uninitialized then we output "(null)" now instead of "(Unspecified)".

If we were to include or exclude peers with an unspecified address we won't get what we would get before.


https://gerrit.asterisk.org/#/c/9174/6/pbx/pbx_dundi.c@4827
PS6, Line 4827: 	strcpy(ip, ast_sockaddr_stringify_fmt(&addrs[0], AST_SOCKADDR_STR_ADDR));
Using strcpy() is unsafe as it can overwrite the buffer if the copied string is too big.  You should pass in the size of the ip buffer and use ast_copy_string().


https://gerrit.asterisk.org/#/c/9174/6/pbx/pbx_dundi.c@4954
PS6, Line 4954: 	if (ast_sockaddr_isnull(sin)) { 
red blob


https://gerrit.asterisk.org/#/c/9174/6/pbx/pbx_dundi.c@4955
PS6, Line 4955: 		((struct sockaddr_in *)&sin->ss)->sin_addr.s_addr = INADDR_ANY;
              : 		sin->ss.ss_family = AF_INET;
              : 		sin->len = sizeof(struct sockaddr_in);
Directly setting values in sin seems just wrong.

How about just calling the function below instead?

ast_sockaddr_parse(sin, "0.0.0.0", 0);


https://gerrit.asterisk.org/#/c/9174/6/pbx/pbx_dundi.c@4958
PS6, Line 4958: 		/* handling no bind address but port in config  */
              : 		if (last_port ==0) {
              : 			ast_sockaddr_set_port(sin, DUNDI_PORT);
              : 			last_port = ast_sockaddr_port(sin);
              : 		} else {
              : 			ast_sockaddr_set_port(sin, last_port);
              : 		}
Pull this out of the if block and put it after.

if (ast_sockaddr_isnull()) {
}
if (last_port == 0) {
   ast_sockaddr_set_port(sin, DUNDI_PORT);
   last_port = ast_sockaddr_port(sin);
} else {
   ast_sockaddr_set_port(sin, last_port);
}


https://gerrit.asterisk.org/#/c/9174/6/pbx/pbx_dundi.c@4967
PS6, Line 4967: 	if (!ast_sockaddr_port(sin)) {
              : 		ast_sockaddr_set_port(sin, DUNDI_PORT);
              : 		last_port = ast_sockaddr_port(sin);
              : 	}
This isn't right.  With the change above it isn't needed either.


https://gerrit.asterisk.org/#/c/9174/6/pbx/pbx_dundi.c@5046
PS6, Line 5046: 	if (set_config("dundi.conf", &sin, 1))
You need to initialize sin with ast_sockaddr_setnull().



-- 
To view, visit https://gerrit.asterisk.org/9174
To unsubscribe, or for help writing mail filters, 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: 6
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-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Wed, 11 Jul 2018 21:35:46 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180711/813d621e/attachment.html>