<p style="white-space: pre-wrap; word-wrap: break-word;">Thanks Joshua.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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).</p><p style="white-space: pre-wrap; word-wrap: break-word;">Kind Regards,<br>Jaco</p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13362">View Change</a></p><p>9 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c">File res/res_rtp_asterisk.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3034">Patch Set #5, Line 3034:</a> <code style="font-family:monospace,monospace"> static struct ast_sockaddr lo6 = { .len = 0 };</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This can be manipulated by multiple threads at once, doing the parse at module startup would be bett […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Moved this into global space, which I don't particularly like but you're right about the threading issue.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3056">Patch Set #5, Line 3056:</a> <code style="font-family:monospace,monospace"> for (ia = ifa; ia && count < PJ_ICE_MAX_CAND; ia = ia->ifa_next) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think all of this new logic could use comments to describe precisely what is going on at each poin […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Tried to add as I see fit, hope the level of is OK now?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3057">Patch Set #5, Line 3057:</a> <code style="font-family:monospace,monospace"> if (!ia->ifa_addr || (ia->ifa_flags & IFF_UP) == 0)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">With coding guidelines this should have {} brackets, same for other places.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3065">Patch Set #5, Line 3065:</a> <code style="font-family:monospace,monospace"> if (ast_sockaddr_isnull(&tmp))</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Under what case would this be NULL?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">So you're suggesting simply do ast_sockaddr_from_sockaddr(...) and move on?</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Updated as per what I think you were trying to suggest.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3096">Patch Set #5, Line 3096:</a> <code style="font-family:monospace,monospace"> if (ast_sockaddr_cmp(&candidate->local, &tmp) == 0) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">In Asterisk we just use !ast_sockaddr_cmp</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Updated in a few other places too.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3108">Patch Set #5, Line 3108:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">If a match occurs this should break out and not continue iterating</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3113">Patch Set #5, Line 3113:</a> <code style="font-family:monospace,monospace"> if (ast_sockaddr_isnull(&tmp) || count >= PJ_ICE_MAX_CAND)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">It shouldn't be possible for count to be > than PJ_ICE_MAX_CAND, if that happens then badness will o […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Updated to ==</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3157">Patch Set #5, Line 3157:</a> <code style="font-family:monospace,monospace"> ast_sockaddr_to_pj_sockaddr(addr, &base);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Does this present a behavior change at all with choosing the base based on addr?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">How critical is this? I can put some effort in to recheck the pjproject ordering against this if crucial.</p><p style="white-space: pre-wrap; word-wrap: break-word;">There is possibly *one* use case where this might change:</p><p style="white-space: pre-wrap; word-wrap: break-word;">ip ad ad 192.168.0.2/24 dev eth0<br>ip ad ad public/32 dev lo<br>ip ro ad default via 192.168.0.1 src public</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3165">Patch Set #5, Line 3165:</a> <code style="font-family:monospace,monospace"> if (!baseset && ast_sockaddr_is_ipv4(&candidate->address) && !ast_sockaddr_is_any(&candidate->address)) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Is it actually possible for a candidate to be any? I don't think that's valid for an ICE candidate</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is srflx - I did my absolute best to clone this functionality "exactly as is currently".</p><p style="white-space: pre-wrap; word-wrap: break-word;">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</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Hmm, come to look at it again ... it looks like I may have messed this up.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/13362">change 13362</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/13362"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-Change-Id: I109eaffc3e2b432f00bf958e3caa0f38cacb4edb </div>
<div style="display:none"> Gerrit-Change-Number: 13362 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: Jaco Kroon <jaco@uls.co.za> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 26 Mar 2020 20:47:57 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>