<p> Attention is currently required from: Sean Bright, Joshua Colp, Joe. </p>
<p>Patch set 7:<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/+/16160">View Change</a></p><p>6 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/7a10ccb2_1bd7761b">Patch Set #7, Line 18:</a> <code style="font-family:monospace,monospace">remove_exising</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/remove_exising/remove_existing</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/c9d68664_623536fa">Patch Set #7, Line 209:</a> <code style="font-family:monospace,monospace">return unreachable ? 0 : CMP_MATCH;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If you only want to select unreachable contacts shouldn't this "match" on unreachable then? i.e. return unreachable ? CMP_MATCH : 0</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/97559334_30134be9">Patch Set #7, Line 491:</a> <code style="font-family:monospace,monospace"> int remove_unavailable = NULL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Initialize this to zero instead of NULL</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/547b1640_0172b083">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 style="white-space: pre-wrap; word-wrap: break-word;">Just a suggestion but I think this can be simplified a bit in order to make the code a little more readable and efficient by initializing a 'res' variable, removing some stack variables, avoiding double NULL checks via cleanup, and short circuiting lookups if a status does not exist.</p><p style="white-space: pre-wrap; word-wrap: break-word;">For example:</p><p style="white-space: pre-wrap; word-wrap: break-word;">int res = ast_tvcmp(left...,right...)<br>left_status = get_left_status<br>if !left_status: return res<br>right_status = get_right_status<br>if !right_status: ao2_ref(left_status, -1); return res<br># now you know both are not null so ...<br>if left_status == unavail: res = -1<br>else if right_status == unavail: res = 1<br>ao2_ref(right/left statuses, -1)<br>return res</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/7a2e92ca_a39b0742">Patch Set #7, Line 718:</a> <code style="font-family:monospace,monospace">ast_sip_location_retrieve_aor_contacts_filtered(aor, AST_SIP_CONTACT_FILTER_UNREACHABLE);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This function returns all un-expired, unreachable contacts, both dynamic and permanent. I don't think you want to include permanent contacts in the count? I think you want to do a filter of the prior retrieved "existing_contacts" (registered non-permanent ones)</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also if keeping this can return NULL so add a NULL check just in case.</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/9fb4b6af_d90f1d25">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 style="white-space: pre-wrap; word-wrap: break-word;">I would first check if remove_unavailable is set, and removing will affect change. If so then I'd remove unavailable to make room, and then retrieve the count again and check to see if we are still in excess. That way if an error occurred during removal (for some reason couldn't be removed) and count still exceeds then it'll fall into the error section and return.</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: Joe <ynadiv@corpit.xyz> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 10 Aug 2021 23:30:05 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>