<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/8374">View Change</a></p><p>Patch set 9:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(13 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/8374/9//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8374/9//COMMIT_MSG@14">Patch Set #9, Line 14:</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;">A minor addition to res_pjsip_session is needed in order to fetch<br>the active SIP dialog.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This sentence is no longer needed.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c">File res/res_pjsip_notify.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@302">Patch Set #9, Line 302:</a> <code style="font-family:monospace,monospace"> char *channel_name;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is not needed. You should get the channel name using the session pointer when needed:<br>ast_channel_name(data->session->channel)</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@409">Patch Set #9, Line 409:</a> <code style="font-family:monospace,monospace"> ast_free(data->channel_name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Remove channel_name</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@468">Patch Set #9, Line 468:</a> <code style="font-family:monospace,monospace"> const char *channel_name, struct ast_sip_session *session, void *info)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Remove channel_name as it is not needed.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@478">Patch Set #9, Line 478:</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;"> data->channel_name = ast_strdup(channel_name);<br> if (!data->channel_name) {<br> ao2_ref(data, -1);<br> return NULL;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">remove</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@740">Patch Set #9, Line 740:</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;"> struct pjsip_dialog *dlg = data->session->inv_session->dlg;<br><br> ast_debug(1, "Sending notify on channel %s\n", data->channel_name);<br><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to recheck if the call is up before getting or using dlg. The call may get hungup between pushing the task and it running now. However, it cannot change while this function is running under the sessions serializer thread.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Should only allow the NOTIFY if the state is PJSIP_INV_STATE_CONFIRMED. Other states are either too early in the dialog establishment to send a NOTIFY or too late and the dialog is gone.</p><p style="white-space: pre-wrap; word-wrap: break-word;">struct pjsip_dialog *dlg;</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (!data->session->channel<br> || !data->session->inv_session<br> || session->inv_session->state != PJSIP_INV_STATE_CONFIRMED) {<br> return -1;<br>}</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">ast_debug(1, "Sending notify on channel %s\n",<br> ast_channel_name(data->session->channel));</pre><p style="white-space: pre-wrap; word-wrap: break-word;">dlg = data->session->inv_session->dlg;</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@745">Patch Set #9, Line 745:</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;"> ast_log(LOG_NOTICE, "Unable to create request to channel %s",<br> data->channel_name);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This log message is redundant as ast_sip_create_request() has already logged a warning.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@754">Patch Set #9, Line 754:</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;"> ast_log(LOG_NOTICE, "Unable to send request to channel %s\n",<br> data->channel_name);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Redundant. ast_sip_send_request() has already logged a warning.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@842">Patch Set #9, Line 842:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_NOTICE, "No channel found with name %s", channel_name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">There should not be a log message if the channel isn't found. It is a valid occurrence if the channel goes away before you can send the NOTIFY message.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@847">Patch Set #9, Line 847:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_NOTICE, "Channel was a non-PJSIP channel: %s\n", channel_name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should be made a warning like other PJSIP specific dialplan functions do.</p><p style="white-space: pre-wrap; word-wrap: break-word;">"Cannot use AMI PJSIPNotify action on a non-PJSIP channel: %s\n"</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@848">Patch Set #9, Line 848:</a> <code style="font-family:monospace,monospace"> return INVALID_CHANNEL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ch is ref leaked here.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@855">Patch Set #9, Line 855:</a> <code style="font-family:monospace,monospace"> if (!session || !session->inv_session || session->inv_session->state == PJSIP_INV_STATE_DISCONNECTED) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should only allow the NOTIFY if the state is PJSIP_INV_STATE_CONFIRMED. Other states are either too early in the dialog establishment to send a NOTIFY or too late and the dialog is gone.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@856">Patch Set #9, Line 856:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_NOTICE, "No active session for channel %s\n", channel_name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should be removed or at most a debug message as this is essentially the same condition as not finding the channel above.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/8374">change 8374</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/8374"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: If7f3151a6d633e414d5dc319d5efc1443c43dd29 </div>
<div style="display:none"> Gerrit-Change-Number: 8374 </div>
<div style="display:none"> Gerrit-PatchSet: 9 </div>
<div style="display:none"> Gerrit-Owner: Nathan Bruning <nathan@iperity.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-Reviewer: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: lvl <digium@lvlconsultancy.nl> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 10 Apr 2018 18:30:59 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>