<p> Attention is currently required from: Sean Bright, Joshua Colp, Kevin Harwell. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/16160">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">Commit Message:</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/+/16160/comment/e2b07571_f9d2b3e1">Patch Set #7, Line 18:</a> <code style="font-family:monospace,monospace">remove_exising</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">s/remove_exising/remove_existing</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_pjsip/location.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/+/16160/comment/0bd67680_5bd8f516">Patch Set #7, Line 209:</a> <code style="font-family:monospace,monospace">return unreachable ? 0 : CMP_MATCH;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">If you only want to select unreachable contacts shouldn't this "match" on unreachable then? i.e. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I know, that confused me too. The callback that calls this function is removing anything with CMP_MATCH, so we don't want to match the unavailable. See the preceding function to this one.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I rewrote the brief, and also changed this to:<br>return !unreachable? CMP_MATCH : 0;</p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm leaving this as unresolved pending your feedback as this function is an exact duplicate of the previous function but flips the return. This can be done with an arg to avoid code duplication, but makes the function harder to read (to me).</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_pjsip_registrar.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/+/16160/comment/bb4736b9_563bf6de">Patch Set #7, Line 491:</a> <code style="font-family:monospace,monospace"> int remove_unavailable = NULL;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Initialize this to zero instead of NULL</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/+/16160/comment/00035e84_f8506220">Patch Set #7, Line 505:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* If contact is unreachable move to top of list */<br> left_status = ast_sip_get_contact_status(left_contact);<br> right_status = ast_sip_get_contact_status(right_contact);<br> if (left_status && right_status) {<br> left_unreachable = (left_status->status == UNAVAILABLE);<br> right_unreachable = (right_status->status == UNAVAILABLE);<br> ao2_cleanup(left_status);<br> ao2_cleanup(right_status);<br> if (left_unreachable == right_unreachable) {<br> /* Either both available or both unavailable */<br> /* Sort from soonest to expire to last to expire */<br> return ast_tvcmp(left_contact->expiration_time, right_contact->expiration_time);<br> }<br> if (left_unreachable) return -1;<br> if (right_unreachable) return 1;<br> }<br><br> /* No status from one or both contacts, return by expiration */<br> ao2_cleanup(left_status);<br> ao2_cleanup(right_status);<br> return ast_tvcmp(left_contact->expiration_time, right_contact->expiration_time);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Just a suggestion but I think this can be simplified a bit in order to make the code a little more r […]</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/+/16160/comment/e911f386_00e4ab72">Patch Set #7, Line 721:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* Check to see if removing those will not help */<br> if (contact_count - unavail_count > aor->max_contacts || !aor->remove_unavailable) {<br> /* Enforce the maximum number of contacts */<br> ast_sip_report_failed_acl(endpoint, rdata, "registrar_attempt_exceeds_maximum_configured_contacts");<br> ast_log(LOG_WARNING, "Registration attempt from endpoint '%s' (%s:%d) to AOR '%s' will exceed max contacts of %u\n",<br> ast_sorcery_object_get_id(endpoint), rdata->pkt_info.src_name, rdata->pkt_info.src_port,<br> aor_name, aor->max_contacts);<br> response->code = 403;<br> pjsip_endpt_release_pool(ast_sip_get_pjsip_endpoint(), details.pool);<br> ao2_cleanup(unavail_contacts);<br> ao2_cleanup(existing_contacts);<br> return;<br> }<br><br> /* Remove any unreachable contacts */<br> remove_excess_contacts(unavail_contacts, contacts, contact_count - aor->max_contacts, aor->remove_existing);<br> ao2_cleanup(unavail_contacts);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I would first check if remove_unavailable is set, and removing will affect change. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/16160">change 16160</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/+/16160"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: Ia2711b08f2b4d1177411b1be23e970d7fdff5784 </div>
<div style="display:none"> Gerrit-Change-Number: 16160 </div>
<div style="display:none"> Gerrit-PatchSet: 7 </div>
<div style="display:none"> Gerrit-Owner: Joe <ynadiv@corpit.xyz> </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-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Gerrit-Attention: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Gerrit-Attention: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Attention: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 11 Aug 2021 16:50:11 +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: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>