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

[Joe]

Attention is currently required from: Joshua Colp, George Joseph, 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 11:

(2 comments)

File res/res_pjsip_registrar.c:

https://gerrit.asterisk.org/c/asterisk/+/16160/comment/abce0780_d4114d19 
PS11, Line 733: 			remove_excess_contacts(unavail_contacts, contacts, contact_count - aor->max_contacts, aor->remove_existing);
> If for some reason 'unavail_contacts' is NULL here (which it could because 'ast_sip_location_retriev […]
We have a null check at line 726, and if it fails unavail_count is 0 at line 731, so this branch will not be executed.


https://gerrit.asterisk.org/c/asterisk/+/16160/comment/724879a2_8ea69a61 
PS11, Line 736: 			contacts = ast_sip_location_retrieve_aor_contacts_nolock(aor);
              : 			if (!contacts) {
              : 				response->code = 500;
              : 				pjsip_endpt_release_pool(ast_sip_get_pjsip_endpoint(), details.pool);
              : 				return;
              : 			}
> Were you able to determine why the contacts are not being removed from the 'contacts' container in t […]
Agreed that this is hacky.

I based this idea of the original version of the remove_existing code before this commit:

https://github.com/asterisk/asterisk/commit/cbf082ed53aa669b999ba57b80966e3382d773d4

I cannot figure out why contacts is not updating as it really is supposed to.  I hate to ask, but since @jcolp wrote the commit above, maybe he can shed some light on why this might not work?  If I can figure this part out it will fix multiple potential issues.

As far the the unref, contacts gets unref'd at the end of this function (keep in mind that contacts already exists here, we're just refreshing it).

Your second point is valid but I don't know of another way to grab the contacts once they've been changed.

A concern I had is what would happen if max_contacts=1 and remove_unavail=yes, then after the (only) contact is removed we'll return a 500.



-- 
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: 11
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-CC: George Joseph <gjoseph at digium.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: George Joseph <gjoseph at digium.com>
Gerrit-Attention: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Thu, 23 Sep 2021 01:45:47 +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/20210922/1d14d311/attachment.html>