[Asterisk-code-review] res_pjsip_registrar: Remove unavailable contacts if exceeds max_contacts (asterisk[16])

[Joe]

Attention is currently required from: Sean Bright, Joshua Colp, Kevin Harwell.
Joe 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:

(5 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/16160/comment/e2b07571_f9d2b3e1 
PS7, Line 18: remove_exising
> s/remove_exising/remove_existing
Done


File res/res_pjsip/location.c:

https://gerrit.asterisk.org/c/asterisk/+/16160/comment/0bd67680_5bd8f516 
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. […]
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.

I rewrote the brief, and also changed this to:
return !unreachable? CMP_MATCH : 0;

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).


File res/res_pjsip_registrar.c:

https://gerrit.asterisk.org/c/asterisk/+/16160/comment/bb4736b9_563bf6de 
PS7, Line 491: 	int remove_unavailable = NULL;
> Initialize this to zero instead of NULL
Done


https://gerrit.asterisk.org/c/asterisk/+/16160/comment/00035e84_f8506220 
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 r […]
Done


https://gerrit.asterisk.org/c/asterisk/+/16160/comment/e911f386_00e4ab72 
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. […]
Done



-- 
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: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Wed, 11 Aug 2021 16:50:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210811/128aa086/attachment-0001.html>