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

[Jaco Kroon]

Jaco Kroon 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:

(9 comments)

Thanks Joshua.

Items I've marked resolved is fixed locally.  Would just like to deploy the update to our QA environment and take it for a test drive, so will probably only push the updates tomorrow evening (probably your morning).

Kind Regards,
Jaco

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 bett […]
Moved this into global space, which I don't particularly like but you're right about the threading issue.


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 poin […]
Tried to add as I see fit, hope the level of is OK now?


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.
Done


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?
So you're suggesting simply do ast_sockaddr_from_sockaddr(...) and move on?

setnull shouldn't be required either if success can be guaranteed, which I don't see how this can fail, but in general I try to play it safe.

Updated as per what I think you were trying to suggest.


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
Updated in a few other places too.


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
Done


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 o […]
Updated to ==


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?
Not as far as I can determine no.  Old code scanned through the returned list from pjproject, which used the exact same ordering from getifaddrs as this code does.  Well, it iterates twice, once for IPv6 and once for IPv4, I iterate once, so IPv4 and IPv6 may come interleaved, but the ordering of v4 and v6 addresses individually should remain the same, as such the base address should not change.

How critical is this?  I can put some effort in to recheck the pjproject ordering against this if crucial.

There is possibly *one* use case where this might change:

ip ad ad 192.168.0.2/24 dev eth0
ip ad ad public/32 dev lo
ip ro ad default via 192.168.0.1 src public

Which I'm fairly certain is considered absolutely insane.  Anyone that currently does that probably have breakage anyway so the change could improve things for them.  Anybody that has a public on eth interfaces is highly unlikely to have that on loopback.


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
This is srflx - I did my absolute best to clone this functionality "exactly as is currently".

Note original line 3097.  If you advise me this should change, I'm happy to make the change on that advise.  This comes from commit 1a349d832d139744658e3d8ab63d8bd0103a3d6f on master, ASTERISK-27437.  https://gerrit.asterisk.org/c/asterisk/+/7331

rtp_add_candidates_to_ice receives addr as an argument, two of the three callers passes thi sas &rtp->rtcp->us, but the third (line 3682 on master in function ice_create passes this as addr which it receives, as &rtp->bind_address, which based on what I know from some unexpected experiences with chan_pjsip, can indeed be any.

Hmm, come to look at it again ... it looks like I may have messed this up.



-- 
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 20:47:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200326/106fc7ba/attachment-0001.html>