[Asterisk-code-review] A dialplan function to change endpoint's configuration on a session (asterisk[13])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/13501 )

Change subject: A dialplan function to change endpoint's configuration on a session
......................................................................


Patch Set 4: Code-Review-1

(6 comments)

https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/chan_pjsip.c 
File channels/chan_pjsip.c:

https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/chan_pjsip.c@2944 
PS4, Line 2944: 		ast_log(LOG_WARNING, "Unable to register PJSIP_ASYMMETRIC_RTP_CODEC dialplan function\n");
s/PJSIP_ASYMMETRIC_RTP_CODEC/PJSIP_SESSION_ENDPOINT


https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/pjsip/dialplan_functions.c 
File channels/pjsip/dialplan_functions.c:

https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/pjsip/dialplan_functions.c@85 
PS4, Line 85: 		Set PJSIP endpoint options flag on a session.
Remove the word "flag" here.


https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/pjsip/dialplan_functions.c@89 
PS4, Line 89: 	<description>
            : 		<para>When written, sets the endpoint option flag of a session. </para>
            : 		<example title="To control asymmetric_rtp_codec">
            : 		 exten => s,n,Set(PJSIP_SESSION_ENDPOINT(asymmetric_rtp_codec)=yes)
            : 		</example>
            : 	</description>
Make a note this should only be used for outgoing calls, used in a pre-dial handler.


https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/pjsip/dialplan_functions.c@1354 
PS4, Line 1354: 	data->session->endpoint = endpoint_copy;
How safe is this? Before this happens are there any instances of the endpoint being used, or pointed to by other things? If so it might be possible for those items to still point to the old endpoint object.

Worse case they didn't bump the ref when that happened, so potential for a crash. Best case they will be pointing to a "stale" object.


https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/pjsip/dialplan_functions.c@1389 
PS4, Line 1389: 
Updating a session's endpoint information only makes sense for outgoing calls, so I'd put a check in to make sure this is for outgoing only.

You should be able to check the role (PJSIP_ROLE_UAC vs PJSIP_ROLE_UAS) on the session's pjsip session:

session->inv_session->role;


https://gerrit.asterisk.org/c/asterisk/+/13501/4/channels/pjsip/dialplan_functions.c@1390 
PS4, Line 1390: 	return ast_sip_push_task_wait_serializer(channel->session->serializer, session_endpoint_write_cb, &edata);
Before calling the write function you also might want to create a list of configuration options that shouldn't be updated (ones used in ast_sip_session_create_outgoing for instance), then check to make sure those are not being set.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13501
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: I600d5ac1c6c2b47a7a0da6425aea559f1aa61c7c
Gerrit-Change-Number: 13501
Gerrit-PatchSet: 4
Gerrit-Owner: Salah Ahmed <txrubel at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Tue, 14 Jan 2020 19:14:40 +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/20200114/9cd769cb/attachment.html>