[Asterisk-code-review] res/res pjsip sip message: added sip message event notifier (asterisk[master])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/10715 )

Change subject: res/res_pjsip_sip_message: added sip message event notifier
......................................................................


Patch Set 1: Code-Review-1

(5 comments)

https://gerrit.asterisk.org/#/c/10715/1//COMMIT_MSG
Commit Message:

https://gerrit.asterisk.org/#/c/10715/1//COMMIT_MSG@9
PS1, Line 9: This module is sending an event messages for a given channel's SIP
What is the use case or reason for this? ARI itself isn't really made or designed for acting as a logging mechanism for SIP.


https://gerrit.asterisk.org/#/c/10715/1/res/res_pjsip_sip_message.c
File res/res_pjsip_sip_message.c:

https://gerrit.asterisk.org/#/c/10715/1/res/res_pjsip_sip_message.c@22
PS1, Line 22: 	<support_level>core</support_level>
This should be extended support level, and disabled by default. This incurs a performance penalty, and most individuals would never use it.


https://gerrit.asterisk.org/#/c/10715/1/res/res_pjsip_sip_message.c@68
PS1, Line 68: static struct ast_sip_session_supplement supp_sip_in_req = {
            : 		.priority = AST_SIP_SUPPLEMENT_PRIORITY_CHANNEL + 1,
            : 		.incoming_request = incoming_request_handler,
            : };
            : 
            : static struct ast_sip_session_supplement supp_sip_in_res = {
            : 		.priority = AST_SIP_SUPPLEMENT_PRIORITY_CHANNEL + 1,
            : 		.incoming_response = incoming_response_handler,
            : };
            : 
            : static struct ast_sip_session_supplement supp_sip_out_req = {
            : 		.priority = AST_SIP_SUPPLEMENT_PRIORITY_CHANNEL + 1,
            : 		.outgoing_request = outgoing_request_handler,
            : };
            : 
            : static struct ast_sip_session_supplement supp_sip_out_res = {
            : 		.priority = AST_SIP_SUPPLEMENT_PRIORITY_CHANNEL + 1,
            : 		.outgoing_response = outgoing_response_handler,
            : };
These can all be combined into a single supplement.


https://gerrit.asterisk.org/#/c/10715/1/res/res_pjsip_sip_message.c@136
PS1, Line 136: 	if ((!session) || (!tdata)) {
             : 		ast_log(LOG_WARNING, "Wrong input parameters.\n");
             : 		return;
             : 	}
             : 
             : 	/* check message type. */
             : 	if (tdata->msg->type != PJSIP_REQUEST_MSG) {
             : 		ast_log(LOG_ERROR, "Wrong message type. It should not reach to here.\n");
             : 		return;
             : 	}
The API provides a guarantee that these can't happen. Same applies elsewhere.


https://gerrit.asterisk.org/#/c/10715/1/res/res_pjsip_sip_message.c@247
PS1, Line 247: 	snapshot = ast_channel_snapshot_create(session->channel);
There is a large penalty for creating a channel snapshot. Instead the current snapshot on the channel should be retrieved and used.



-- 
To view, visit https://gerrit.asterisk.org/10715
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dedcc678265ebb74d35e7a2f58bc4b5084bcfe1
Gerrit-Change-Number: 10715
Gerrit-PatchSet: 1
Gerrit-Owner: sungtae kim <pchero21 at gmail.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Comment-Date: Tue, 27 Nov 2018 13:52:37 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181127/cf78e579/attachment.html>