[asterisk-dev] [Code Review] Don't set up a call forward when receiving a 482 Loop Detected

[Mark Michelson]

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

Ship it!


Aside from the cosmetic issue below, this looks like a fine change. Good job, Terry!


/branches/1.4/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/764/#comment5135>

    Nitpick: The comment does not line up with the other comments in this section.


- Mark


On 2010-07-06 16:02:13, Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/764/
> -----------------------------------------------------------
> 
> (Updated 2010-07-06 16:02:13)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Summary
> -------
> 
> I have noticed an issue where if we make an outbound SIP call which is responded to with a 482 Loop Detected, instead of failing the call and letting the dialplan handle the error, we instead set chan->call_forward to a Local channel at the callers context with the extension that they were dialing. I can't think of a reason why we would want to do this. It makes it impossible to handle the error in any meaningful way. It appears Asterisk has done this since r3662:
> 
> $ svn log -r 3662
> ------------------------------------------------------------------------
> r3662 | markster | 2004-08-26 20:16:16 -0700 (Thu, 26 Aug 2004) | 2 lines
> 
> When detecting a hairpin, redirect to the appropriate local extension (bug #1974)
> 
> 
> (bug #1974 doesn't seem to be related, btw)
> 
> The code also has the note:
>                    /* \note SIP is incapable of performing a hairpin call, which
>                    is yet another failure of not having a layer 2 (again, YAY
>                     IETF for thinking ahead).  So we treat this as a call
>                     forward and hope we end up at the right place... */
> 
> which is clearly incorrect as RFC 3261 talks about how to handle spiralin, but if a *loop* (not a spiral) is detected by the destination, it is an error condition and I think should be treated as such. It is always "fun" changing behavior that "has always been there" but in my opinion this is just clearly wrong behavior.
> 
> This patch for 1.4 adds the sip option forwardloopdetected which defaults to yes (the old behavior). For trunk, I would just delete the existing behavior.
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/channels/chan_sip.c 274242 
>   /branches/1.4/configs/sip.conf.sample 274242 
> 
> Diff: https://reviewboard.asterisk.org/r/764/diff
> 
> 
> Testing
> -------
> 
> I have tested that the options are properly set and reported via 'sip show settings' and 'sip show peer'. Dialing out and receiving a 482 with no option set results in the old behavior and with it set results in immediately continuing in the dialplan with HANGUPCAUSE=127 and DIALSTATUS=CONGESTION.
> 
> 
> Thanks,
> 
> Terry
> 
>