Review Board 1.7.16


Add Path header support to chan_sip

Review Request #991 - Created Oct. 29, 2010 and submitted

Klaus Darilion
1.8
18223
Reviewers
asterisk-dev
Asterisk
This patch adds support for the Parse header (RFC 3327). This allows to record the Path (route) of incoming REGISTER requests and then reuse the learned path on outgoing out-of-dialog requests as a pre-loaded route set.

I tried to reuse as much of existing code as possible. There are several parts where I am not sure if it is implemented nice and follow chan_sip design rules (are there any?) I will post my questions as a review to myself.
Basic testing done (with nat=yes|no, with and without outboundproxy set for a peer, with and without Parse header, single Parse header and two Parse headers, restarting Asterisk)
Posted (Oct. 29, 2010, 9:06 a.m.)

   

  
/branches/1.8/channels/chan_sip.c (Diff revision 1)
 
 
How does realtime work? How do I defined the fields which should be saved/read from DB?
  1. It's basically a name=value concept, much like the configurations. Just add a field. The important part here is that we need an option on whether to have Path support in realtime or not, otherwise you are forcing EVERYONE to change their database tables.
/branches/1.8/channels/chan_sip.c (Diff revision 1)
 
 
Here is reuse this "private" function to parse the first hop and set the destination. Should I make the function "public" (remove __ and put the definition on top of the file)?
/branches/1.8/channels/chan_sip.c (Diff revision 1)
 
 
I used a separate DB entry as adding the Path to SIP/Registry would make parsing of the astDB result more complicated. Since Russel+Stephan speed up astDB I think that should be fine.
  1. I think we should have ONE entry per registration, not multiple. If the parsing is messy, it's time to solve that issue instead of bypassing it and adding a new entry to the soup.
/branches/1.8/channels/chan_sip.c (Diff revision 1)
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
Probably this is not a nice solution: I just copied the sip_proxy chain elements from peer to pvt. Any hints?
/branches/1.8/channels/chan_sip.c (Diff revision 1)
 
 
more or less a copy from build_route(). Probably such functions should be made more generic to allow code reuse.
/branches/1.8/channels/chan_sip.c (Diff revision 1)
 
 
I guess this one could be removed
/branches/1.8/channels/chan_sip.c (Diff revision 1)
 
 
I guess this one could be removed too
/branches/1.8/channels/chan_sip.c (Diff revision 1)
 
 
 
 
I would like to make there also a "general" option for usepath. How to do it?
Is there a rule how to use use the flags? Looks like bit 27 was not used and I just used the first empty bit.
Posted (Nov. 18, 2010, 8:51 p.m.)
A first set of comments from a hotel in Germany.
/branches/1.8/channels/chan_sip.c (Diff revision 1)
 
 
Or correctly spelled. "Updating". It should be "ast_debug(2," instead of ast_log(LOG_DEBUG
/branches/1.8/channels/chan_sip.c (Diff revision 1)
 
 
Try to make it "Path header" instead. "Use path" doesn't mean much to people but if you refer to a header, they can guess a bit more.
/branches/1.8/channels/chan_sip.c (Diff revision 1)
 
 
 
 
Just check the reset of the code in there. It's not hard.

I would prefer "pathsupport" or something else than "use"...
/branches/1.8/configs/sip.conf.sample (Diff revision 1)
 
 
It's not "router" header, it's "Route: header"

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.