<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/1082/">https://reviewboard.asterisk.org/r/1082/</a>
     </td>
    </tr>
   </table>
   <br />





<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
 <p style="margin-top: 0;">On January 24th, 2011, 1:34 p.m., <b>rmudgett</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;">I don&#39;t see anything more to really comment about.</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;">Thanks for the review!</pre>
<br />





<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
 <p style="margin-top: 0;">On January 24th, 2011, 1:34 p.m., <b>rmudgett</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/1082/diff/3/?file=15278#file15278line734" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/features.c</a>
    <span style="font-weight: normal;">

     (Diff revision 3)

    </span>
   </th>
  </tr>
 </thead>

 <tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
  <tr>

   <td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static void check_goto_on_transfer(struct ast_channel *chan)</pre></td>

  </tr>
 </tbody>



 
 




 <tbody>

  <tr>
    <th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">734</font></th>
    <td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">        xferchan-&gt;_softhangup = 0;</pre></td>
    <th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">734</font></th>
    <td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">        ast_channel_clear_softhangup(xferchan, AST_SOFTHANGUP_ALL);</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;">I don&#39;t know why this line was even here to begin with.  I don&#39;t think that _softhangup will ever be nonzero here.</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;">Well, I think I&#39;d like to leave it, and here&#39;s why:

1) If it&#39;s not needed, it&#39;s harmless (just a little bit of extra work in the middle of an already expensive operation (transfer)).

2) It is needed for some reason we can&#39;t think of, so instead of breaking it to find that out, we just leave it and let it keep working.  :-)</pre>
<br />




<p>- Russell</p>


<br />
<p>On January 24th, 2011, 1:07 p.m., Russell Bryant wrote:</p>






<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.orgrb/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.</div>
<div>By Russell Bryant.</div>


<p style="color: grey;"><i>Updated 2011-01-24 13:07:03</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;">Mantis issue #18585 reports that a channel redirect out of MeetMe() stopped working properly.  This issue includes a patch that resolves the issue by removing a call to ast_check_hangup() from app_meetme.c.  I left that in my patch, as it doesn&#39;t need to be there.  However, the rest of the patch fixes this problem with or without the change to app_meetme.

The key difference between what happens before and after this patch is the effect of the END_OF_Q control frame.  After END_OF_Q is hit in ast_read(), ast_read() will return NULL.  With the ast_check_hangup() removed, app_meetme sees this which causes it to exit as intended.  Checking ast_check_hangup() caused app_meetme to exit earlier in the process, and the target of the redirect saw the condition where ast_read() returned NULL.

Removing ast_check_hangup() works around the issue in app_meetme, but doesn&#39;t solve the issue if another application did the same thing.  There are also other edge cases where if an application finishes at the same time that a redirect happens, the target of the redirect will think that the channel hung up.  So, I made some changes in pbx.c to resolve it at a deeper level.  There are already places that unset the SOFTHANGUP_ASYNCGOTO flag in an attempt to abort the hangup process.  My patch extends this to remove the END_OF_Q frame from the channel&#39;s read queue, making the &quot;abort hangup&quot; more complete. </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;">I replicated the bug easily.  I verified that removing ast_check_hangup() fixes the issue and determined why it fixes it.  I also verified that the issue is resolved with the pbx.c changes, both with and without ast_check_hangup() still used in app_meetme.</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/view.php?id=18585">18585</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/apps/app_meetme.c <span style="color: grey">(303508)</span></li>

 <li>/branches/1.8/include/asterisk/channel.h <span style="color: grey">(303508)</span></li>

 <li>/branches/1.8/main/channel.c <span style="color: grey">(303508)</span></li>

 <li>/branches/1.8/main/features.c <span style="color: grey">(303508)</span></li>

 <li>/branches/1.8/main/pbx.c <span style="color: grey">(303508)</span></li>

</ul>

<p><a href="https://reviewboard.asterisk.org/r/1082/diff/" style="margin-left: 3em;">View Diff</a></p>




  </td>
 </tr>
</table>








  </div>
 </body>
</html>