<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1749/">https://reviewboard.asterisk.org/r/1749/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 16th, 2012, 10:33 a.m., <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On February 17th, 2012, 7:26 a.m., <b>kwemheuer</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On February 17th, 2012, 8:27 a.m., <b>wdoekes</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.)</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- kwemheuer</p>
<br />
<p>On February 16th, 2012, 3:58 p.m., schmidts wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers and wdoekes.</div>
<div>By schmidts.</div>
<p style="color: grey;"><i>Updated Feb. 16, 2012, 3:58 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">tested with the sipp scenario from wdoekes (thank you for this).</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-19358">ASTERISK-19358</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>branches/1.8/channels/chan_sip.c <span style="color: grey">(355573)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1749/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>