<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/3990/">https://reviewboard.asterisk.org/r/3990/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 16th, 2014, 12:21 p.m. CDT, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/3990/diff/1/?file=67328#file67328line1312" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/main/dial.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1312</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1313</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ds</span><span class="o">-></span><span class="n">consumed</span> <span class="o">=</span> <span class="mi">1</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1314</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1315</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ast_channel_cleanup</span><span class="p">(</span><span class="n">ds</span><span class="o">-></span><span class="n">peer_chan</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1316</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ds</span><span class="o">-></span><span class="n">peer_chan</span> <span class="o">=</span> <span class="nb">NULL</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Instead of setting the consume flag, remove the datastore and free it:
if (!ast_channel_datastore_remove(old_chan, ds)) {
ast_datastore_free(ds);
}
Note that it is safe to do that here, as the list traversal in channel.c assumes that this can occur.
That also removes the need for de-refing the peer_chan here, as your destructor will take care of it for you.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Unfortunately, ds is a pointer to a datastore's data and not to the data itself. I'll go ahead and rename the variable to make that clearer. In the meantime I guess I could try to fetch the datastore in the normal manner and destroy it that way.</pre>
<br />
<p>- Jonathan</p>
<br />
<p>On September 11th, 2014, 4:59 p.m. CDT, Jonathan Rose wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, Matt Jordan and rmudgett.</div>
<div>By Jonathan Rose.</div>
<p style="color: grey;"><i>Updated Sept. 11, 2014, 4:59 p.m.</i></p>
<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-24237">ASTERISK-24237</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<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;">Reproduction:
pj123 calls 1601
pj123 does a SIP blonde transfer to 1603
1603 answers
FRACK occurs
all are PJSIP endpoints.
Basically what happens is there is a second outbound dial from another pj123 channel. Before the dial is answered, the pj123 gets masqueraded out of the picture and replaced with a channel that isn't in the dial state.
This patch fixes that by advancing the incoming channel to the dial state in the channel breakdown function of a datastore on the pj123 channel. Honestly, I'm not sure this is entirely adequate, but it seems to work in all of the conditions I've tried so far and it doesn't impede normal attended transfers. I might need to try and figure out what happens if I masquerade in a channel that is already dialing though. I'm not sure if that's something we can really expect to happen under normal conditions, but that seems like something that would screw up this approach.
Note that I think this patch is the right idea, I just don't know if I need to account for the possibility that the channel that masquerades into pj123's dialing channel might not be in the neutral state.</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;">Ran against reproduction of the above scenario. Assertion was gone and the CDR results were as follows:
"","123","1601","default",""""" <123>","PJSIP/pj123-00000000","PJSIP/1601-00000001","Dial","PJSIP/1601,,tT","2014-09-11 21:48:51","2014-09-11 21:48:53","2014-09-11 21:48:57",5,4,"ANSWERED","DOCUMENTATION","1410472131.0",""
"","123","","default",""""" <123>","PJSIP/pj123-00000002","PJSIP/1603-00000003","Dial","PJSIP/1603","2014-09-11 21:48:55",,"2014-09-11 21:48:57",2,0,"NO ANSWER","DOCUMENTATION","1410472135.6",""
"","1601","1603","default",""""" <1601>","PJSIP/1601-00000001","PJSIP/1603-00000003","AppDial","(Outgoing Line)","2014-09-11 21:48:57","2014-09-11 21:48:57","2014-09-11 21:49:04",6,6,"ANSWERED","DOCUMENTATION","1410472131.1",""
And dial events reported by AMI:
http://pastebin.com/tWuwL7xa
(note that there is a mismatch between the number of dial end and dial begin events... not sure if this is a problem, but I might be able to fix it just by updating the old chan, not sure what status code to use though).
Ran against normal attended transfer, feature attended transfers, and blind transfers with no noticeable effect.
Ran against testsuite blonde transfer tests, some attended transfer tests, some blind transfer tests, and all pjsip transfer tests (at least ones that will run on my box... a few won't due to sipp version requirements that I really need to get around to fixing eventually) for comparison purposes. All passed exhibiting the same behavior as before as far as warning messages and such go.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/12/main/stasis_channels.c <span style="color: grey">(422882)</span></li>
<li>/branches/12/main/dial.c <span style="color: grey">(422882)</span></li>
<li>/branches/12/include/asterisk/dial.h <span style="color: grey">(422882)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3990/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>