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

[George Joseph]

> On Sept. 17, 2013, 1:22 p.m., Mark Michelson wrote:
> > /branches/12/res/res_pjsip_header_funcs.c, line 246
> > <https://reviewboard.asterisk.org/r/2858/diff/1/?file=45986#file45986line246>
> >
> >     See https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines#CodingGuidelines-Codeformatting
> >     
> >     While it's not explicit there, it does state that spacing should be consistent throughout the codebase. This means that you should put spaces around the '+' sign here.
> >     
> >     Also, it's unclear why you're comparing to headerlen + 2. headerlen is the length of the header name (or partial header name) passed into PJSIP_HEADER. l is the total length of the header. Is the + 2 supposed to account for the : and null byte?

Was trying to insure that the returned string had at least the ':' and 1 extra character for a value.  Cheap test but if a header can have an empty value, then not needed.


> On Sept. 17, 2013, 1:22 p.m., Mark Michelson wrote:
> > /branches/12/res/res_pjsip_header_funcs.c, lines 325-334
> > <https://reviewboard.asterisk.org/r/2858/diff/1/?file=45986#file45986line325>
> >
> >     You can save yourself some allocations (which, admittedly are cheap when using pj_pool_t) by reformatting this as:
> >     
> >     pj_str_t pj_header_name;
> >     pj_str_t pj_header_value;
> >     
> >     pj_cstr(&pj_header_name, hname);
> >     pj_cstr(&pj_header_value, hvalue);
> >     
> >     le->hdr = pjsip_generic_string_hdr_create(pool, hname, hvalue);

Ok, wasn't sure to what lengths you guys go to limit stack usage.  


> On Sept. 17, 2013, 1:22 p.m., Mark Michelson wrote:
> > /branches/12/res/res_pjsip_header_funcs.c, lines 211-213
> > <https://reviewboard.asterisk.org/r/2858/diff/1/?file=45986#file45986line211>
> >
> >     See https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines#CodingGuidelines-Declarationoffunctionsandvariables
> >     
> >     Don't declare variables mid-block. This one can actually be enforced by the compiler by running Asterisk's configure script with --enable-dev-mode. This way, warnings are treated as errors, so you will be unable to compile the code with these variable declarations here.
> >     
> >     Same comment applies to other places where you have declared variables mid-block.

Unfortunately, there are other places in that fail with --enable-dev-mode.  :)


- George


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


On Sept. 15, 2013, 6:34 p.m., George Joseph wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2858/
> -----------------------------------------------------------
> 
> (Updated Sept. 15, 2013, 6:34 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/20130917/76f95cfb/attachment-0001.htm>