[Asterisk-code-review] res pjsip notify.c: enable in-dialog NOTIFY (asterisk[15])

[Sean Bright]

Sean Bright has posted comments on this change. ( https://gerrit.asterisk.org/8374 )

Change subject: res_pjsip_notify.c: enable in-dialog NOTIFY
......................................................................


Patch Set 1: Code-Review-1

(13 comments)

https://gerrit.asterisk.org/#/c/8374/1/include/asterisk/res_pjsip.h
File include/asterisk/res_pjsip.h:

https://gerrit.asterisk.org/#/c/8374/1/include/asterisk/res_pjsip.h@3112
PS1, Line 3112: struct pjsip_dialog* ast_sip_get_dialog_for_channel(struct ast_channel* ch);
Coding guidelines: The '*' should have a space before it, not after it.


https://gerrit.asterisk.org/#/c/8374/1/res/res_pjsip.c
File res/res_pjsip.c:

https://gerrit.asterisk.org/#/c/8374/1/res/res_pjsip.c@4746
PS1, Line 4746: struct pjsip_dialog* ast_sip_get_dialog_for_channel(struct ast_channel* ch) {
Coding guidelines: The '*' should have a space before it, not after it.


https://gerrit.asterisk.org/#/c/8374/1/res/res_pjsip.c@4753
PS1, Line 4753: 	struct ast_sip_channel_pvt *channel = ast_channel_tech_pvt(ch);
              : 	struct ast_sip_session *session = channel->session;
These declarations need to moved to the top of their enclosing block. The assignments can remain here if needed.


https://gerrit.asterisk.org/#/c/8374/1/res/res_pjsip_notify.c
File res/res_pjsip_notify.c:

https://gerrit.asterisk.org/#/c/8374/1/res/res_pjsip_notify.c@68
PS1, Line 68: 			If <literal>URI</literal> is used, the	default outbound endpoint will be used
One too many spaces between 'the' and 'default'


https://gerrit.asterisk.org/#/c/8374/1/res/res_pjsip_notify.c@302
PS1, Line 302: 	struct ast_channel* channel;
Coding guidelines: The '*' should have a space before it, not after it.


https://gerrit.asterisk.org/#/c/8374/1/res/res_pjsip_notify.c@465
PS1, Line 465: static struct notify_channel_data* notify_ami_channel_data_create(
             : 		struct ast_channel* channel, void *info)
Coding guidelines: The '*' should have a space before it, not after it.


https://gerrit.asterisk.org/#/c/8374/1/res/res_pjsip_notify.c@734
PS1, Line 734: 	ast_log(LOG_DEBUG, "Sending notity on channel %s\n", ast_channel_name(data->channel));
Use ast_debug() instead (and 'notify' is misspelled)


https://gerrit.asterisk.org/#/c/8374/1/res/res_pjsip_notify.c@738
PS1, Line 738: 	ast_log(LOG_DEBUG, "Channel has active dialog: %p\n", dlg);
Use ast_debug()


https://gerrit.asterisk.org/#/c/8374/1/res/res_pjsip_notify.c@742
PS1, Line 742: 		ast_log(LOG_WARNING, "SIP NOTIFY - No active dialog for channel\n");
Drop the "SIP NOTIFY - " from all of these warnings & errors. Also, are all of these off-nominal paths really something that the user needs to be aware of via warnings/errors?


https://gerrit.asterisk.org/#/c/8374/1/res/res_pjsip_notify.c@781
PS1, Line 781: 	struct ast_channel* channel, void *info);
Coding guidelines: The '*' should have a space before it, not after it.


https://gerrit.asterisk.org/#/c/8374/1/res/res_pjsip_notify.c@839
PS1, Line 839: 	// note: this increases the refcount of the channel
Don't use C++ style comments


https://gerrit.asterisk.org/#/c/8374/1/res/res_pjsip_notify.c@840
PS1, Line 840: 	struct ast_channel* ch = ast_channel_get_by_name(channel_name);
Coding guidelines: The '*' should have a space before it, not after it.


https://gerrit.asterisk.org/#/c/8374/1/res/res_pjsip_notify.c@842
PS1, Line 842: 		ast_log(LOG_WARNING, "No channel found with name %s", channel_name);
Does this really need to be logged? If not, consider making it an ast_debug().



-- 
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: 1
Gerrit-Owner: Nathan Bruning <nathan at iperity.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Comment-Date: Fri, 23 Feb 2018 12:45:03 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180223/942cca99/attachment-0001.html>