[Asterisk-code-review] chan pjsip: add a new function PJSIP DTMF MODE (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/5909 )

Change subject: chan_pjsip: add a new function PJSIP_DTMF_MODE
......................................................................


Patch Set 10: Code-Review-1

(6 comments)

https://gerrit.asterisk.org/#/c/5909/10/channels/pjsip/dialplan_functions.c
File channels/pjsip/dialplan_functions.c:

https://gerrit.asterisk.org/#/c/5909/10/channels/pjsip/dialplan_functions.c@1083
PS10, Line 1083: int pjsip_acf_dtmf_mode_write(struct ast_channel *chan, const char *cmd, char *data, const char *value)
Dynamically changing DTMF mode usually requires a SIP reinvite to let the remote device know the new DTMF behavior.

* Incoming calls could change the mode before the call is answered without a reinvite.
* Outgoing calls will require a reinvite once the call is answered.  This situation can happen using the AMI Setvar action with the new dialplan function after the call is originated but before it is answered.
* After the call is answered a reinvite is required.

A call to ast_sip_session_refresh() will handle most of the above cases.  It does need to be executed under the channel's serializer.  See pjsip_acf_media_offer_write() for an example call to ast_sip_push_task_synchronous().


https://gerrit.asterisk.org/#/c/5909/10/channels/pjsip/dialplan_functions.c@1113
PS10, Line 1113: 	channel->session->dtmf = dtmf;
If the dtmf mode does not change then we should exit early.


https://gerrit.asterisk.org/#/c/5909/10/channels/pjsip/dialplan_functions.c@1116
PS10, Line 1116: 	if (pjsip_pvt->media && pjsip_pvt->media[SIP_MEDIA_AUDIO]  && (pjsip_pvt->media[SIP_MEDIA_AUDIO])->rtp) {
pjsip_pvt->media is not a pointer but an array name.  Array names are never NULL.


https://gerrit.asterisk.org/#/c/5909/10/channels/pjsip/include/dialplan_functions.h
File channels/pjsip/include/dialplan_functions.h:

https://gerrit.asterisk.org/#/c/5909/10/channels/pjsip/include/dialplan_functions.h@113
PS10, Line 113: #endif /* _PJSIP_DIALPLAN_FUNCTIONS */
Why is this line changed?


https://gerrit.asterisk.org/#/c/5909/10/res/res_pjsip.c
File res/res_pjsip.c:

https://gerrit.asterisk.org/#/c/5909/10/res/res_pjsip.c@4397
PS10, Line 4397: 		strncpy(buf, "none", buf_len);
Use ast_copy_string() instead of strncpy() in this function.

strncpy() has a few undesired side effects.  If buf_len is shorter than the string to copy, then buf is left unterminated.  If buf_len is longer than the string then all extra space in the buf is filled with a null.


https://gerrit.asterisk.org/#/c/5909/10/res/res_pjsip.c@4421
PS10, Line 4421: enum ast_sip_dtmf_mode ast_sip_str_to_dtmf(const char * dtmf_mode)
               : {
               : 	int result = -1;
Mixing an enum and an int can have undesired side effects. Since -1 is not a member of the enum you are in undefined territory.



-- 
To view, visit https://gerrit.asterisk.org/5909
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I20eef5da3e5d1d3e58b304416bc79683f87e7612
Gerrit-Change-Number: 5909
Gerrit-PatchSet: 10
Gerrit-Owner: Torrey Searle <tsearle at gmail.com>
Gerrit-Reviewer: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Fri, 07 Jul 2017 17:27:11 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170707/e5769ced/attachment.html>