Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/8374 )
Change subject: res_pjsip_notify.c: enable in-dialog NOTIFY
......................................................................
Patch Set 9: Code-Review-1
(13 comments)
https://gerrit.asterisk.org/#/c/8374/9//COMMIT_MSG
Commit Message:
https://gerrit.asterisk.org/#/c/8374/9//COMMIT_MSG@14
PS9, Line 14: A minor addition to res_pjsip_session is needed in order to fetch
: the active SIP dialog.
This sentence is no longer needed.
https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c
File res/res_pjsip_notify.c:
https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@302
PS9, Line 302: char *channel_name;
This is not needed. You should get the channel name using the session pointer when needed:
ast_channel_name(data->session->channel)
https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@409
PS9, Line 409: ast_free(data->channel_name);
Remove channel_name
https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@468
PS9, Line 468: const char *channel_name, struct ast_sip_session *session, void *info)
Remove channel_name as it is not needed.
https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@478
PS9, Line 478: data->channel_name = ast_strdup(channel_name);
: if (!data->channel_name) {
: ao2_ref(data, -1);
: return NULL;
: }
remove
https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@740
PS9, Line 740: struct pjsip_dialog *dlg = data->session->inv_session->dlg;
:
: ast_debug(1, "Sending notify on channel %s\n", data->channel_name);
:
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.
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.
struct pjsip_dialog *dlg;
if (!data->session->channel
|| !data->session->inv_session
|| session->inv_session->state != PJSIP_INV_STATE_CONFIRMED) {
return -1;
}
ast_debug(1, "Sending notify on channel %s\n",
ast_channel_name(data->session->channel));
dlg = data->session->inv_session->dlg;
https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@745
PS9, Line 745: ast_log(LOG_NOTICE, "Unable to create request to channel %s",
: data->channel_name);
This log message is redundant as ast_sip_create_request() has already logged a warning.
https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@754
PS9, Line 754: ast_log(LOG_NOTICE, "Unable to send request to channel %s\n",
: data->channel_name);
Redundant. ast_sip_send_request() has already logged a warning.
https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@842
PS9, Line 842: ast_log(LOG_NOTICE, "No channel found with name %s", channel_name);
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.
https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@847
PS9, Line 847: ast_log(LOG_NOTICE, "Channel was a non-PJSIP channel: %s\n", channel_name);
This should be made a warning like other PJSIP specific dialplan functions do.
"Cannot use AMI PJSIPNotify action on a non-PJSIP channel: %s\n"
https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@848
PS9, Line 848: return INVALID_CHANNEL;
ch is ref leaked here.
https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@855
PS9, Line 855: if (!session || !session->inv_session || session->inv_session->state == PJSIP_INV_STATE_DISCONNECTED) {
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.
https://gerrit.asterisk.org/#/c/8374/9/res/res_pjsip_notify.c@856
PS9, Line 856: ast_log(LOG_NOTICE, "No active session for channel %s\n", channel_name);
This should be removed or at most a debug message as this is essentially the same condition as not finding the channel above.
--
To view, visit https://gerrit.asterisk.org/8374
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: If7f3151a6d633e414d5dc319d5efc1443c43dd29
Gerrit-Change-Number: 8374
Gerrit-PatchSet: 9
Gerrit-Owner: Nathan Bruning <nathan at iperity.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: lvl <digium at lvlconsultancy.nl>
Gerrit-Comment-Date: Tue, 10 Apr 2018 18:30:59 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180410/47931496/attachment.html>