[asterisk-dev] [Code Review] 2466: Pimp my SIP: Call forwarding & diversion headers

[Joshua Colp]

> On April 25, 2013, 3:41 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/res/res_sip_diversion.c, line 205
> > <https://reviewboard.asterisk.org/r/2466/diff/2/?file=36393#file36393line205>
> >
> >     Having this return a value is silly as it only ever returns 1.
> 
> Kevin Harwell wrote:
>     This function implements the ast_sip_session_supplement function pointer incoming_request which expects a return value.  Did you mean change it on the supplement struct and for all supplements?

I'm silly, again. Drop it.


> On April 25, 2013, 3:41 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/res/res_sip_diversion.c, lines 241-248
> > <https://reviewboard.asterisk.org/r/2466/diff/2/?file=36393#file36393line241>
> >
> >     Did you swipe this from pjsip source code?
> 
> Kevin Harwell wrote:
>     Ooops yeah forgot about that.  Can I use it and add a comment or do I need to do something else?

It just stood out. A comment is fine.


> On April 25, 2013, 3:41 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/include/asterisk/res_sip.h, lines 331-332
> > <https://reviewboard.asterisk.org/r/2466/diff/2/?file=36390#file36390line331>
> >
> >     chan_sip has an option called "promiscredir" which follows the explicit SIP URI and does not just go to the user portion within the dialplan. Is this something we want to do as well here? (albeit maybe not the same way)
> 
> Kevin Harwell wrote:
>     I can add this option...and a call to ast_channel_call_forward_build right?  Or is that what you meant by not the same way?

Yeah. The conundrum for supporting it is how to expose it to the dialplan. Right now in chan_sip if you have the option enabled it sends a call to SIP/<the URI, minus the sip: at the front> - This won't really work since it has no endpoint and using the dialed endpoint seems wrong. How about exposing the SIP URI via a dialplan variable and sending the call to an explicit extension?

ie: If enabled all diverted calls go to the "diverted" extension and the user can then decide what to do with the SIP URI they have been diverted to.

Thoughts?


- Joshua


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


On April 24, 2013, 8:09 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2466/
> -----------------------------------------------------------
> 
> (Updated April 24, 2013, 8:09 p.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp and Mark Michelson.
> 
> 
> Bugs: ASTERISK-21426
>     https://issues.asterisk.org/jira/browse/ASTERISK-21426
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Adds call forwarding support (Josh's patch) to the new SIP work being done in Asterisk.  This also includes the ability to add a diversion header to an outgoing response/request when appropriate.  The diversion header feature can be turned off by setting the send_diversion=false (defaults to true) on an endpoint within the configuration file.
> 
> 
> Diffs
> -----
> 
>   /team/group/pimp_my_sip/channels/chan_gulp.c 386451 
>   /team/group/pimp_my_sip/include/asterisk/res_sip.h 386451 
>   /team/group/pimp_my_sip/res/res_sip.c 386451 
>   /team/group/pimp_my_sip/res/res_sip/sip_configuration.c 386451 
>   /team/group/pimp_my_sip/res/res_sip_diversion.c PRE-CREATION 
>   /team/group/pimp_my_sip/res/res_sip_session.c 386451 
> 
> Diff: https://reviewboard.asterisk.org/r/2466/diff/
> 
> 
> Testing
> -------
> 
> Wrote a few testsuite tests to make sure calls are being forwarded and the diversion header is added/propagated appropriately.
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

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