[Asterisk-code-review] res_pjsip_registrar: Remove unavailable contacts if exceeds max_contacts (asterisk[16])
Kevin Harwell
asteriskteam at digium.com
Tue Aug 10 18:30:05 CDT 2021
Attention is currently required from: Sean Bright, Joshua Colp, Joe.
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16160 )
Change subject: res_pjsip_registrar: Remove unavailable contacts if exceeds max_contacts
......................................................................
Patch Set 7: Code-Review-1
(6 comments)
Commit Message:
https://gerrit.asterisk.org/c/asterisk/+/16160/comment/7a10ccb2_1bd7761b
PS7, Line 18: remove_exising
s/remove_exising/remove_existing
File res/res_pjsip/location.c:
https://gerrit.asterisk.org/c/asterisk/+/16160/comment/c9d68664_623536fa
PS7, Line 209: return unreachable ? 0 : CMP_MATCH;
If you only want to select unreachable contacts shouldn't this "match" on unreachable then? i.e. return unreachable ? CMP_MATCH : 0
File res/res_pjsip_registrar.c:
https://gerrit.asterisk.org/c/asterisk/+/16160/comment/97559334_30134be9
PS7, Line 491: int remove_unavailable = NULL;
Initialize this to zero instead of NULL
https://gerrit.asterisk.org/c/asterisk/+/16160/comment/547b1640_0172b083
PS7, Line 505: /* If contact is unreachable move to top of list */
: left_status = ast_sip_get_contact_status(left_contact);
: right_status = ast_sip_get_contact_status(right_contact);
: if (left_status && right_status) {
: left_unreachable = (left_status->status == UNAVAILABLE);
: right_unreachable = (right_status->status == UNAVAILABLE);
: ao2_cleanup(left_status);
: ao2_cleanup(right_status);
: if (left_unreachable == right_unreachable) {
: /* Either both available or both unavailable */
: /* Sort from soonest to expire to last to expire */
: return ast_tvcmp(left_contact->expiration_time, right_contact->expiration_time);
: }
: if (left_unreachable) return -1;
: if (right_unreachable) return 1;
: }
:
: /* No status from one or both contacts, return by expiration */
: ao2_cleanup(left_status);
: ao2_cleanup(right_status);
: return ast_tvcmp(left_contact->expiration_time, right_contact->expiration_time);
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.
For example:
int res = ast_tvcmp(left...,right...)
left_status = get_left_status
if !left_status: return res
right_status = get_right_status
if !right_status: ao2_ref(left_status, -1); return res
# now you know both are not null so ...
if left_status == unavail: res = -1
else if right_status == unavail: res = 1
ao2_ref(right/left statuses, -1)
return res
https://gerrit.asterisk.org/c/asterisk/+/16160/comment/7a2e92ca_a39b0742
PS7, Line 718: ast_sip_location_retrieve_aor_contacts_filtered(aor, AST_SIP_CONTACT_FILTER_UNREACHABLE);
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)
Also if keeping this can return NULL so add a NULL check just in case.
https://gerrit.asterisk.org/c/asterisk/+/16160/comment/9fb4b6af_d90f1d25
PS7, Line 721: /* Check to see if removing those will not help */
: if (contact_count - unavail_count > aor->max_contacts || !aor->remove_unavailable) {
: /* Enforce the maximum number of contacts */
: ast_sip_report_failed_acl(endpoint, rdata, "registrar_attempt_exceeds_maximum_configured_contacts");
: ast_log(LOG_WARNING, "Registration attempt from endpoint '%s' (%s:%d) to AOR '%s' will exceed max contacts of %u\n",
: ast_sorcery_object_get_id(endpoint), rdata->pkt_info.src_name, rdata->pkt_info.src_port,
: aor_name, aor->max_contacts);
: response->code = 403;
: pjsip_endpt_release_pool(ast_sip_get_pjsip_endpoint(), details.pool);
: ao2_cleanup(unavail_contacts);
: ao2_cleanup(existing_contacts);
: return;
: }
:
: /* Remove any unreachable contacts */
: remove_excess_contacts(unavail_contacts, contacts, contact_count - aor->max_contacts, aor->remove_existing);
: ao2_cleanup(unavail_contacts);
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.
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/16160
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Ia2711b08f2b4d1177411b1be23e970d7fdff5784
Gerrit-Change-Number: 16160
Gerrit-PatchSet: 7
Gerrit-Owner: Joe <ynadiv at corpit.xyz>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Joe <ynadiv at corpit.xyz>
Gerrit-Comment-Date: Tue, 10 Aug 2021 23:30:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210810/6babacab/attachment-0001.html>
More information about the asterisk-code-review
mailing list