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

[George Joseph]

> On Oct. 3, 2013, 12:33 p.m., Matt Jordan wrote:
> > /branches/12/res/res_pjsip_header_funcs.c, lines 54-60
> > <https://reviewboard.asterisk.org/r/2858/diff/3/?file=46938#file46938line54>
> >
> >     It may be worthwhile adding a <note> to this that states that reading PJSIP_HEADER with the remove option will still remove the headers. That can be done before commit however.
> 
> George Joseph wrote:
>     Just confirming... the same thread that PJSIP_HEADER will be called from will also call the supplemental session callbacks?  If you allow me to say "Matt said so" I'll remove the locks. :)
>     
>     Since I don't have commit access, I'll add the note, fix the formatting and re-upload the patch.  I ran indent and didn't see anything obvious in the formatting so let me know what should be fixed.
> 
> Joshua Colp wrote:
>     Oh now I understand why you lock. Locking in the PJSIP architecture is so uncommon. Yeah, this code may work as-is but it's not obeying the approach set out. Operations exist as tasks which are pushed onto a task queue for the session itself. This ensures that operations for the session are serialized, and also guarantees that multiple things aren't operating on a session at the same time. media_offer_write would be a good thing to use as a basis.
> 
> George Joseph wrote:
>     The purpose of the lock was to keep the dialplan function from manipulating the header list at the same time as the supplemental callbacks.  
>     
>     On an incoming channel I still have to use the supp callback and a list because I don't see any way to retrieve an arbitrary header other than in a supp callback.  They're not saved anywhere. I suppose that the incoming callback will always be executed before any dialplan function could be so I could remove the locking.
>     
>     As for outgoing... media_offer_write has the luxury of having specific well-known structures to manipulate (req_caps and override_prefs) so it can save it's data directly in the session any time the dialplan function is called and the rest of the stack knows what to do with it.  Unless I'm missing something, at the time a pre-dial-handler is called, pjsip_hdr hasn't been created yet so there's no place to store arbitrary headers and no code anywhere that would know what to do with them.  Hence accumulating headers in a session datastore and having the outgoing supp callback add them to the pjsip_msg.  Again, if there's no possibility of the dialplan function running at the same time as the callback, I'll remove the locking.
>     
>
> 
> Joshua Colp wrote:
>     I'm not referring to how you do it (list) but where you do it. While a lock does provide the guarantee that two threads can't access it this approach isn't used in the PJSIP architecture overall. Stuff is queued as a task and only one task can execute on the session at a time, this doesn't require a lock for the list but still guarantees that no other thread will access it at the same time. Instead of introducing locking and blurring the line I'd prefer to continue using the task approach.

Ok, I *think* what you're saying is...
Remove the locking altogether.
In the read and write dialplan function implementations, use ast_sip_push_task_synchronous to call read_header, add_header, update_header and remove_header.

Yes?

I'm assuming that the supp callbacks are already executed as tasks and don't need to have ast_sip_push_task_synchronous called on them again?


- George


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


On Sept. 30, 2013, 3:15 p.m., George Joseph wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2858/
> -----------------------------------------------------------
> 
> (Updated Sept. 30, 2013, 3:15 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/20131003/c88d3ebf/attachment.html>