[Asterisk-code-review] res_rtp_asterisk: iterate all local addresses looking to populate ICE. (asterisk[13])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/13362 )

Change subject: res_rtp_asterisk: iterate all local addresses looking to populate ICE.
......................................................................


Patch Set 5: Code-Review-1

(9 comments)

https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c 
File res/res_rtp_asterisk.c:

https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3034 
PS5, Line 3034: 	static struct ast_sockaddr lo6 = { .len = 0 };
This can be manipulated by multiple threads at once, doing the parse at module startup would be better.


https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3056 
PS5, Line 3056: 		for (ia = ifa; ia && count < PJ_ICE_MAX_CAND; ia = ia->ifa_next) {
I think all of this new logic could use comments to describe precisely what is going on at each point for better understanding


https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3057 
PS5, Line 3057: 			if (!ia->ifa_addr || (ia->ifa_flags & IFF_UP) == 0)
With coding guidelines this should have {} brackets, same for other places.


https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3065 
PS5, Line 3065: 			if (ast_sockaddr_isnull(&tmp))
Under what case would this be NULL?


https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3096 
PS5, Line 3096: 				if (ast_sockaddr_cmp(&candidate->local, &tmp) == 0) {
In Asterisk we just use !ast_sockaddr_cmp


https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3108 
PS5, Line 3108: 
If a match occurs this should break out and not continue iterating


https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3113 
PS5, Line 3113: 			if (ast_sockaddr_isnull(&tmp) || count >= PJ_ICE_MAX_CAND)
It shouldn't be possible for count to be > than PJ_ICE_MAX_CAND, if that happens then badness will occur


https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3157 
PS5, Line 3157: 				ast_sockaddr_to_pj_sockaddr(addr, &base);
Does this present a behavior change at all with choosing the base based on addr?


https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3165 
PS5, Line 3165: 				if (!baseset && ast_sockaddr_is_ipv4(&candidate->address) && !ast_sockaddr_is_any(&candidate->address)) {
Is it actually possible for a candidate to be any? I don't think that's valid for an ICE candidate



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13362
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: I109eaffc3e2b432f00bf958e3caa0f38cacb4edb
Gerrit-Change-Number: 13362
Gerrit-PatchSet: 5
Gerrit-Owner: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Thu, 26 Mar 2020 13:18:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200326/8781317a/attachment.html>