[asterisk-dev] [Code Review] 2858: res_pjsip_header_funcs: New module to create PJSIP_HEADER, PJSIPAddHeader and PJSIPRemoveHeader

Wed Sep 18 12:05:50 CDT 2013

> On Sept. 17, 2013, 7:22 p.m., Mark Michelson wrote:
> > Thanks for the submission! Despite the number of comments below, this is a really great first effort. Most of the comments below focus on small matters that can be easily corrected, such as coding guidelines violations. I'll take another pass at this once you fix the stuff mentioned here.
> 
> Olle E Johansson wrote:
>     I like having this functionality, but based on experiences would like to suggest name changes. The current one in chan_sip only adds headers to the initial INVITE so it's really ADDHEADERINVITEREQUEST. We need to have a name that also allow for adding functions to add headers to responses and possibly BYE requests and responses. THis just copies the chan_sip names which many years of experience has told me was a bad choice of names. This early in the morning I'm unable to come up with a good suggestion, so I leave that to someone else.
> 
> Mark Michelson wrote:
>     I have a couple of ideas about this.
>     
>     1.
>     
>     Matt had an idea on the dev list to wrap the functionality of PJSIP_HEADER, PJSIPAddHeader, and PJSIPRemoveHeader into a single function, 
>     PJSIP_HEADER. When used in read mode (e.g. Set(MYVAR=${PJSIP_HEADER(CSeq)})), the function would work like it currently does.
>     
>     When used in write mode, though, you could do something like:
>     
>     Set(PJSIP_HEADER(X-Custom-Header)=Hello)
>     
>     in order to add "X-Customer-Header: Hello" to the outbound INVITE. You could do
>     
>     Set(PJSIP_HEADER(X-Custom-Header)=)
>     
>     in order to remove the X-Custom-Header from the outbound INVITE [1].
>     
>     We could expand upon this some to incorporate your idea without the need to add more functions. The PJSIP_HEADER function could have a second parameter that states what methods the header should be added to. For instance:
>     
>     Set(PJSIP_HEADER(X-Custom-Header, BYE)=GoodBye)
>     
>     would place "X-Custom-Header: GoodBye" onto BYE requests on the dialog. If no method is provided, we could interpret a default value of "INVITE" since this is the previous behavior of the SIPAddHeader function.
>     
>     This would at least provide a way of adding custom headers to various requests within the dialog. Responses would be more difficult, since presumably you may have separate custom headers that you want for provisional responses, 200 OKs, and other final responses. You may also have different custom headers you would want to use for different request methods. However, some syntax could be worked out that would allow for the appropriate level of customization.
>     
>     [1] This method of removing headers would not allow for setting headers with no value, which is strange but allowable AFAICT under RFC 3261's ABNF (depending on the header type). 
>     
>     2.
>     
>     We could keep the PJSIP_HEADER function, and PJSIPAddHeader and PJSIPRemoveHeader application syntax, but change their intended usage. Currently, as you have pointed out, they only apply to INVITEs, and the functions are intended to be used in the same dialplan snippets that handle incoming calls.
>     
>     Session supplements have the ability to be called into when any request or response is sent or received during an INVITE-intiated dialog. A session supplement could call a configured dialplan gosub when a request or response is received or sent. Within this gosub, a user could use PJSIPAddHeader and PJSIPRemoveHeader in order to add or remove headers from the request or response that is about to be sent out. They could use PJSIP_HEADER in order to read the headers in the current request or response that is being sent/received.
>     
>     
>     Honestly, approach 2 sounds really cool to me, but it may be too much of a departure from what people are used to. It also, unfortunately, would require some significant rework from George on this patch. Approach 1 is less dynamic, but it is also more familiar. It also means that the initial work George is doing here would be valid; we just would need to expand on it once the initial groundwork is laid.
>     
>     
>     What do you think?
> 
> George Joseph wrote:
>     My 2 cents...  This sounds like an opportunity to do the right thing for the future so don't worry about the work on my part.  I've got the time and can dig in when you guys decide on the direction.
>

Well, *I* obviously like my idea :-)

I think option #2 is neat, but honestly, it isn't precluded by option #1. Having dialplan callbacks that called when certain things happen in Asterisk is something we've talked about from time to time. Using PJSIP_HEADER in such a callback would just change what request the function applies to by default.

I'd say go for option #1, and let's keep option #2 on the table as a good improvement to go do.

Olle: What do you think?

> On Sept. 17, 2013, 7:22 p.m., Mark Michelson wrote:
> > /branches/12/res/res_pjsip_header_funcs.c, lines 4-6
> > <https://reviewboard.asterisk.org/r/2858/diff/1/?file=45986#file45986line4>
> >
> >     Heh, feel free to put yourself as the Copyright holder and author. Credit where credit is due :)
> 
> Olle E Johansson wrote:
>     Credits used to go into the CREDITS file, not the individual source files. If you want to change that policy, there will be a flood of commits. I don't argue that it's wrong, just that we have had a different policy.
> 
> Mark Michelson wrote:
>     Yes, unfortunately, the CREDITS file has fallen into disuse. I know Matt typically uses a script that scans SVN commits in order to determine who contributors to Asterisk are.
>     
>     When it comes to adding a new file, though, it's been policy for as long as I've been on the project to put your own name as the author of the file if you're the original writer. It's a bit odd to give Digium the copyright on the file if someone else created it.

As an aside: I'm always up for adding more people to the CREDITS, or for people to add themselves for individual patches that they write. I don't have an automated way of stripping out subversion commit messages and populating the CREDITS - which would be quite the challenge.

- Matt

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2858/#review9715
-----------------------------------------------------------

On Sept. 17, 2013, 9:35 p.m., George Joseph wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2858/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2013, 9:35 p.m.)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Bugs: ASTERISK-22498
>     https://issues.asterisk.org/jira/browse/ASTERISK-22498
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> For PJSIP_HEADER, an incoming supplemental session callback is registered that takes the pjsip_hdrs from the incoming session and stores them in a linked list in the session datastore.  Calls to PJSIP_HEADER traverse over the list and return the nth matching header where 'n' is the 'number' argument to the function.
> 
> For PJSIPAddHeader, the first call creates a datastore and linked list and adds the datastore to the session.  The header is then created as a pjsip_hdr and added to the list.  An outgoing supplemental session callback then traverses the list and adds the headers to the outgoing pjsip_msg.
> 
> For PJSIPRemoveHeader, the list created with PJSIPAddHeader is traversed and all matching entries are removed.  As with SIPRemoveHeader, an empty arguments removes all headers previously added.
> 
> Rather than cloning the incoming pjsip_msg or using pjsip_msg to accumulate the outgoing headers, I used a simple AST_LIST. There was a lot of overhead with the clone functions and some tricky behavior with the pjsip_msg/pjsip_hdr internal lists. Using the AST_LISTs cut down on both instructions and memory.  
> 
> All memory allocation is from the pj_pool attached to the session.
> 
> 
> Diffs
> -----
> 
>   /branches/12/res/res_pjsip_header_funcs.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2858/diff/
> 
> 
> Testing
> -------
> 
> Tested successfully...
> PJSIP_HEADER return the full value of nth specified header from either the incoming session, or headers previously added to the outgoing session by PJSIPHeader.
> PJSIP_HEADER fails if there was no header specified in the function call.
> PJSIP_HEADER fails if there was no datastore on the incoming session (should never happen).
> PJSIP_HEADER fails if the nth header wasn't found.
> 
> PJSIPAddHeader adds a pjsip_hdr structure to the linked list when the input string is properly formatted as "header_name:\s*header_value".
> PJSIPAddHeader fails if no ':' was found in the input string.
> PJSIPAddHeader fails if after parsing either the header name or value is empty.
> 
> PJSIPRemoveHeader removes all matching headers from the linked list when a partial header name is specified.
> PJSIPRemoveHeader removes all matching headers from the linked list when a full header is specified with a trailing ':'.
> PJSIPRemoveHeader removes all previously added header from the linked list when no header is specified.
> PJSIPRemoveHeader returns successfully (silently) if there was no linked list.
> 
> 
> Thanks,
> 
> George Joseph
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130918/1d6b4112/attachment.htm>