<p style="white-space: pre-wrap; word-wrap: break-word;">Some additional comments. I personally think this should suffice but there are still one or two issues on which I need some guidance that I'm aware of. There may be more.</p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13362">View Change</a></p><p>3 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@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;">Not as far as I can determine no. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Correction: Yes. According to the way I understand RFC 5245 the base address (raddr) should be the address from whence we sent the STUN request, and rport the associated port. So this enforces that in the case where addr for the socket is known (not ANY). Otherwise the old behaviour is followed (which I suspect is wrong, we should use something similar to ast_ouraddrfor towards stunserver (might be good enough) since raddr should be the address we used to send to the STUN server. This is how I understand it, could be wrong, and due to bug mentioned I also understand that the address used here should also be a candidate.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Happy to provide:</p><p style="white-space: pre-wrap; word-wrap: break-word;">1. Exact old behaviour (replace these lines with baseset = 0).<br>2. As is currently (my recommendation).<br>3. ast_outaddrfor (might be faster since we don't iterate the candidate set, but involves multiple system calls).</p><p style="white-space: pre-wrap; word-wrap: break-word;">Your call and I'll abide by what you decide here, since I don't actually use STUN.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Currently, as is, this will be different from current if, and only if, RTC?P is bound to a specific address to begin with (As provided in addr), and that address isn't the first IPv4 address discovered.</p><p style="white-space: pre-wrap; word-wrap: break-word;">There is a further possible change of ordering of ICE candidates in case of use of [ice_host_candidates]. Which does not actually influence base address selection.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The old code got a list of up to 32 addresses from PJPROJECT. Let's say:</p><p style="white-space: pre-wrap; word-wrap: break-word;">1.1.1.1; 1.1.1.2; 1.1.1.3</p><p style="white-space: pre-wrap; word-wrap: break-word;">If we then override 1.1.1.1 to 1.1.1.4 *and* have include_local, the *old* code would have:</p><p style="white-space: pre-wrap; word-wrap: break-word;">1.1.1.4; 1.1.1.2; 1.1.1.3; 1.1.1.1</p><p style="white-space: pre-wrap; word-wrap: break-word;">New code will have:</p><p style="white-space: pre-wrap; word-wrap: break-word;">1.1.1.4; 1.1.1.1; 1.1.1.2; 1.1.1.3</p><p style="white-space: pre-wrap; word-wrap: break-word;">Since both iterate to find the first address both variants should still select 1.1.1.4 as the base addr (in case addr is ANY).</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also, if we end up hitting the 32 limit, the old code would discard early on found addresses, whereas, as per above ordering, I include earlier found addresses and their aliases.</p><p style="white-space: pre-wrap; word-wrap: break-word;">If we ever hit that 32 limit on any system, there is probably something seriously wrong with the system configuration anyway. Whilst I have many addresses, there are only a handful which should be used for rtp in case of addr being "any".</p><p style="white-space: pre-wrap; word-wrap: break-word;">I do not believe that this should present a problem.</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;">This is srflx - I did my absolute best to clone this functionality "exactly as is currently". […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ok, so original line 3097 isn't related to this.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I think you're right, nor would it make sense. Removed.</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@3179">Patch Set #5, Line 3179:</a> <code style="font-family:monospace,monospace"> ast_sockaddr_to_pj_sockaddr(addr, &base);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is a bug. This sets base back to ANY for PJSIP, resulting in raddr being :: in SDP.</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: 6 </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: Fri, 27 Mar 2020 17:07:16 +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"> Comment-In-Reply-To: Jaco Kroon <jaco@uls.co.za> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>