[asterisk-dev] [Code Review]: Fixing the regression from wrong set route information on provisional sip responses

[kwemheuer]

> On Feb. 16, 2012, 10:33 a.m., Mark Michelson wrote:
> > This is definitely the correct approach to take. Don't make the route persistent when building a route set from a provisional response. I have an alternative implementation in my head though. Instead of telling the build_route() function whether to make the route set persistent, just have build_route() automatically do it if the passed-in request is a SIP provisional response. That way, people can't make the mistake of using the wrong value for the persistent parameter. The correct behavior will happen every time.
> 
> kwemheuer wrote:
>     This is my first visit in the reviewboard, so apologies if this is the wrong procedure. The patch fixes one of the problems of ASTERISK-19358 (sending ACK to wrong phone). But the issues original core is "Failed to CANCEL a call in ringing state". The fix for this was the patch from Mark Mitchelson from 14/Feb/12 2:07 PM. I think this should be integrated.
> 
> wdoekes wrote:
>     Mark wrote:
>     > Okay, I'm going to go ahead and commit the contents of the patch. I'll leave this issue open until you tell me I should close it though.
>     
>     But I see he hasn't done that yet.
>     
>     Nevertheless, IMO these are two different bugs, even if they may have been caused by the same changeset (which I don't know if they are).
>     
>     The CANCEL issue is about using routing information when it shouldn't.
>     The ACK issue is about using the wrong routing information.
>     
>     They are related, but not identical. Different bugs call for different patches. (And ideally, different issues on the issue tracker, but we won't bother you with that.)

Yes, You are right that there are two issues: First I stumbled into the CANCEL issue. After posting on the user list I open the issue in jira. While testing Marks solution for this issue, I found the ACK issue. Both issues are related and caused by the same changes. The solution for ASTERISK-19358 is Marks patch. The patch from Stefan solves the ACK issue. The easiest way would be to handle both in this review and integrate the four lines from Mark into the patch. If You want it in a very clear and formal way, there must be a new issue in jira for the ACK issue. But if You do this, you have to set the linking between jira and reviewboard correctly: This review currently handles the ACK issue, the ASTERISK-19358 in jira describes the CANCEL issue. But I may be wrong with this, as this is my first contact with the review board. So apologies if I am wrong.

Nevertheless I am glad to see that there is a solution for the problems.

If I can do anything (open a new ticket in jira, test something etc) let me know.


- kwemheuer


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


On Feb. 16, 2012, 3:58 p.m., schmidts wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1749/
> -----------------------------------------------------------
> 
> (Updated Feb. 16, 2012, 3:58 p.m.)
> 
> 
> Review request for Asterisk Developers and wdoekes.
> 
> 
> Summary
> -------
> 
> by adding the parse_ok_header and build_route for provisional responses to invite. I had caused a regression that asterisk stores a wrong route set from this provisional packets. With this change a route set from a provisionial response will not be stored persistant and will be replaced with the route information from a 200 ok packet.
> see bug ASTERISK-19358 for more information and sip traces.
> 
> 
> This addresses bug ASTERISK-19358.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19358
> 
> 
> Diffs
> -----
> 
>   branches/1.8/channels/chan_sip.c 355573 
> 
> Diff: https://reviewboard.asterisk.org/r/1749/diff
> 
> 
> Testing
> -------
> 
> tested with the sipp scenario from wdoekes (thank you for this).
> 
> 
> Thanks,
> 
> schmidts
> 
>

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