<p style="white-space: pre-wrap; word-wrap: break-word;">I'm going to -1 my own code here, but would like a decision from yourselves as to which route to follow before submitting Patchset 7.</p><p>Patch set 6:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13362">View Change</a></p><p>1 comment:</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;">If it's fixing a bug then I say go that route. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Was originally intended as an optimization (ie, if we have an address available in addr, use it) but after re-reading the RFCs I realized it might actually be fixing a bug as well. I really don't mind here ... happy to follow recommendation here.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The question then becomes - is this a bug or not?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Essentially there are a few things I see here:</p><p style="white-space: pre-wrap; word-wrap: break-word;">1. If addr is not ANY - does it make sense to include any candidates other than addr?</p><p style="white-space: pre-wrap; word-wrap: break-word;">2. Whatever we select for raddr (base) as I understand MUST be a candidate (even though I don't understand WHY), and addr may be ice blacklisted (as used in my code now). The kernel would drop traffic to those addresses anyway, or worse, deliver it to a different process.</p><p style="white-space: pre-wrap; word-wrap: break-word;">3. raddr (base) MUST be IPv4 (since STUN only makes sense in IPv4 world, logically, but IPv6 too has NAT implementations which is outside the scope of this discussion).</p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm however not going to submit as a separate patch simply because I don't use STUN so if it is a bug, it doesn't actually affect me (directly, currently). If this is intended to address a bug fix then I agree, we fork this out onto a separate patch and follow option (3) above. Of course this may require us to include addr as a candidate (even if it is blacklisted).</p><p style="white-space: pre-wrap; word-wrap: break-word;">On that front, if addr is not ANY - does it make sense to add any other discovered addresses as ICE candidates since if they're not bound the kernel will simply drop the traffic?</p><p style="white-space: pre-wrap; word-wrap: break-word;">So yes, perhaps this should split out into a separate patch. Since PJSIP always binds the RTP address to ANY this doesn't affect my use-case but since I'm tampering with the code, I may just as well ...</p><p style="white-space: pre-wrap; word-wrap: break-word;">Your call here. There are two routes here:</p><p style="white-space: pre-wrap; word-wrap: break-word;">(1) Add a check for addr being IPv4 to this snippet (3) above.<br>(2) Remove the snippet completely and fork into separate patch.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm inclined to head to (2) pending discussion on the ML.</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: Jaco Kroon <jaco@uls.co.za> </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 01 Apr 2020 06:37:23 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Comment-In-Reply-To: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Comment-In-Reply-To: Kevin Harwell <kharwell@digium.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>