[Asterisk-code-review] chan sip: Add dialplan function SIP HEADERS (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/6119 )

Change subject: chan_sip: Add dialplan function SIP_HEADERS
......................................................................


Patch Set 2: Code-Review-1

(5 comments)

A note in the CHANGES file is needed to let people know about the new function.

https://gerrit.asterisk.org/#/c/6119/2/channels/chan_sip.c
File channels/chan_sip.c:

https://gerrit.asterisk.org/#/c/6119/2/channels/chan_sip.c@400
PS2, Line 400: 			you may	use <literal>${SIP_HEADERS(X-)}</literal> to enumerate optional extended
Tab in the middle of the line between "may use".


https://gerrit.asterisk.org/#/c/6119/2/channels/chan_sip.c@23092
PS2, Line 23092: 	SCOPED_CHANNELLOCK(chanlock, chan);
               : 
               : 	if (!chan) {
Using SCOPED_CHANNELLOCK() and then testing if chan is NULL doesn't work.  We will segfault on the lock before we test if it is NULL.


https://gerrit.asterisk.org/#/c/6119/2/channels/chan_sip.c@23108
PS2, Line 23108: 	AST_STANDARD_APP_ARGS(args, data);
This modifies the data string.  The usual code copies to a local string before parsing to avoid the side effect:
parse = ast_strdupa(data);
AST_STANDARD_APP_ARGS(args, parse);


https://gerrit.asterisk.org/#/c/6119/2/channels/chan_sip.c@23115
PS2, Line 23115: 		if (ast_begins_with(header, args.pattern)) {
Could check for '*' to mean match any header for compatibility with the REFER match using '*' to match all.  See the REFER review: https://gerrit.asterisk.org/#/c/6118/


https://gerrit.asterisk.org/#/c/6119/2/channels/chan_sip.c@23119
PS2, Line 23119: 				struct ast_str *token = ast_str_alloca(100);
Need to pull the ast_str_alloca() out of the loop because it allocates stack memory for every loop iteration.



-- 
To view, visit https://gerrit.asterisk.org/6119
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2076d3893d03a2f82429f393b5b46db6cf68a267
Gerrit-Change-Number: 6119
Gerrit-PatchSet: 2
Gerrit-Owner: Kirill Katsnelson <kkm at smartaction.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Tue, 01 Aug 2017 18:53:05 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170801/817f22c1/attachment.html>