<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/5909">View Change</a></p><p>Patch set 10:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(6 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/5909/10/channels/pjsip/dialplan_functions.c">File channels/pjsip/dialplan_functions.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5909/10/channels/pjsip/dialplan_functions.c@1083">Patch Set #10, Line 1083:</a> <code style="font-family:monospace,monospace">int pjsip_acf_dtmf_mode_write(struct ast_channel *chan, const char *cmd, char *data, const char *value)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Dynamically changing DTMF mode usually requires a SIP reinvite to let the remote device know the new DTMF behavior.</p><ul><li>Incoming calls could change the mode before the call is answered without a reinvite.</li><li>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.</li><li>After the call is answered a reinvite is required.</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">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().</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5909/10/channels/pjsip/dialplan_functions.c@1113">Patch Set #10, Line 1113:</a> <code style="font-family:monospace,monospace">       channel->session->dtmf = dtmf;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If the dtmf mode does not change then we should exit early.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5909/10/channels/pjsip/dialplan_functions.c@1116">Patch Set #10, Line 1116:</a> <code style="font-family:monospace,monospace">      if (pjsip_pvt->media && pjsip_pvt->media[SIP_MEDIA_AUDIO]  && (pjsip_pvt->media[SIP_MEDIA_AUDIO])->rtp) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">pjsip_pvt->media is not a pointer but an array name.  Array names are never NULL.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5909/10/channels/pjsip/include/dialplan_functions.h">File channels/pjsip/include/dialplan_functions.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5909/10/channels/pjsip/include/dialplan_functions.h@113">Patch Set #10, Line 113:</a> <code style="font-family:monospace,monospace">#endif /* _PJSIP_DIALPLAN_FUNCTIONS */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why is this line changed?</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5909/10/res/res_pjsip.c">File res/res_pjsip.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5909/10/res/res_pjsip.c@4397">Patch Set #10, Line 4397:</a> <code style="font-family:monospace,monospace">            strncpy(buf, "none", buf_len);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Use ast_copy_string() instead of strncpy() in this function.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5909/10/res/res_pjsip.c@4421">Patch Set #10, Line 4421:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">enum ast_sip_dtmf_mode ast_sip_str_to_dtmf(const char * dtmf_mode)<br>{<br>       int result = -1;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/5909">change 5909</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/5909"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I20eef5da3e5d1d3e58b304416bc79683f87e7612 </div>
<div style="display:none"> Gerrit-Change-Number: 5909 </div>
<div style="display:none"> Gerrit-PatchSet: 10 </div>
<div style="display:none"> Gerrit-Owner: Torrey Searle <tsearle@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Alexei Gradinari <alex2grad@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 07 Jul 2017 17:27:11 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>