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

Tue Sep 17 17:17:26 CDT 2013

> On Sept. 17, 2013, 7:22 p.m., Mark Michelson wrote:
> > /branches/12/res/res_pjsip_header_funcs.c, lines 82-83
> > <https://reviewboard.asterisk.org/r/2858/diff/1/?file=45986#file45986line82>
> >
> >     When you say "remove all P- headers" does that mean remove all headers beginning with "P-" or remove all headers with the literal value "P-" will be removed? Based on context clues, I'm thinking that since you did not include a colon, this will remove headers beginning with "P-", but it's not made as clear as it could be in the initial description.
> 
> George Joseph wrote:
>     Not clear in SIPRemoveHeader either which is where I copied this (and a bunch of other stuff) from. :)
>     I'll clean them up.  Want me to update chan_sip with the new language?

Updating chan_sip would be fine, but I'd suggest not doing that on this particular review.

> On Sept. 17, 2013, 7:22 p.m., Mark Michelson wrote:
> > /branches/12/res/res_pjsip_header_funcs.c, lines 164-175
> > <https://reviewboard.asterisk.org/r/2858/diff/1/?file=45986#file45986line164>
> >
> >     The use of rdata->tp_info.pool here worries me. I think that it would be better to use a pool whose lifetime is the same scope as the ast_sip_session. On the dev-list, I pointed out a way to get to the pjsip_dialog's pool. That would be a better choice here. Either that, or the pjsip_inv_session's pool_active.
> 
> George Joseph wrote:
>     Also copied from another module. :)  Do you think I should use the inv_session->pool_active or go all the way down to dlg->pool?

Either the dlg->pool or the inv_session->pool would be fine. They essentially have the same lifetime.

The big thing you have to keep in mind is the lifetime of the data that you are allocating. The thing about the rdata->tp_info.pool is that it gets reset once the rdata is no longer being used. The thread where you are doing this is a different thread from where PJSIP_HEADER() would be called. By the time PJSIP_HEADER is used, the pool may have been reset already. Other places where the rdata->tp_info.pool should be using it with the knowledge that the lifetime of the allocation is not to be trusted from beyond the current thread of execution.

> On Sept. 17, 2013, 7: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.
> 
> George Joseph wrote:
>     Unfortunately, there are other places in that fail with --enable-dev-mode.  :)

Yeah, this was just something that jumped out at me. If there are other places that have failures, then you'll need to be sure to get those fixed up.

> On Sept. 17, 2013, 7: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);
> 
> George Joseph wrote:
>     Ok, wasn't sure to what lengths you guys go to limit stack usage.

Usually the line of thinking is to prefer using the stack over the heap unless the allocation is going to be obviously large, or if the allocation is in a loop. When it comes to something like a pj_str_t, go ahead and use the stack.

> On Sept. 17, 2013, 7:22 p.m., Mark Michelson wrote:
> > /branches/12/res/res_pjsip_header_funcs.c, line 373
> > <https://reviewboard.asterisk.org/r/2858/diff/1/?file=45986#file45986line373>
> >
> >     Instead of checking length and using pj_strnicmp2(), just use pjstricmp()
> 
> George Joseph wrote:
>     I think that because the ':' is part of the supplied value but not part of the header itself I'd have to strdup 'data' and overwrite the ':' with \0 to use a stricmp.  I was just trying to save the copy.

Ah, my mistake.

- Mark

-----------------------------------------------------------
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/20130917/6e267d43/attachment-0001.htm>