<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/1819/">https://reviewboard.asterisk.org/r/1819/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Definitely a good change. I, like you, wonder why ast_check_hangup() does not return true for the ;1 leg of a local channel when it clearly has been hung up. It looks as though chan_local does not do any sort of marking of softhangup on the two channels. I think that a change needs to go into chan_local to mark the softhangup flags on the owner and chan during a local_hangup(). Given, however, that messing with chan_local is like juggling bottles of nitroglycerin, I'd recommend saving such a patch for a later time.</pre>
<br />
<div>
<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/1819/diff/1/?file=26179#file26179line486" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/apps/app_chanspy.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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 int start_spying(struct ast_autochan *autochan, const char *spychan_name, struct ast_audiohook *audiohook)</pre></td>
</tr>
</tbody>
<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">486</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/* Make sure the channel isn't a zombie before we attach the audio hook. */</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">487</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">ast_test_flag</span><span class="p">(</span><span class="n">autochan</span><span class="o">-></span><span class="n">chan</span><span class="p">,</span> <span class="n">AST_FLAG_ZOMBIE</span><span class="p">))</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">488</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</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">489</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <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">490</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think it's definitely correct to be checking the zombie state of a channel within app_chanspy, but I think this should be done sooner. In channel_spy() there is an if block that checks ast_check_hangup() for the spy and spyee's channels. This seems like a good place to do the zombie chec, as well. This way, we don't do any extra work we don't have to (e.g. initializing an audio hook, sending a manager event saying we're spying on a channel).</pre>
</div>
<br />
<p>- Mark</p>
<br />
<p>On March 16th, 2012, 11:33 a.m., jrose 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, Mark Michelson and Matt Jordan.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated March 16, 2012, 11:33 a.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;">The call setup for this bug was sort of elaborate, but roughly 50% of the time, it would create a situation where chanspy can latch onto a zombie channel which would keep the zombie alive forever and keep itself in a perpetual loop. The approach of this patch is to keep that from happening by preventing chanspy from attaching audiohooks to zombies.</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;">The test for this was against a configuration similar to the one described in the bug report.
I started by spying on a local channel...
<extensions.conf>
exten => 038,1,ChanSpy(Local/s@phones,qs)
...
That local channel was a member of a queue and when it picked up, it would dial a SIP peer.
<queues.conf>
[markq]
member => Local/s@phones/n
<extensions.conf>
[phones]
exten => s,1,Dial(SIP/gold)
and I had a sip peer dial into that queue.
<extensions.conf>
exten => 037,1,Queue(markq)
Between all of these, five channels were established:
1 - SIP channel spying on the "local channel"
1 - SIP channel dialing into the queue
1 - SIP channel being dialed by a local channel
and 2 local channels bot with the name s@phones,qs with the difference between them being ;1 and ;2 at the end.
At this point, the chanspy could bind to either channel, and it would screw up as described in the bug report if the spy went to ;2. What would happen is, channel 1 and 2 would hang up, but because they were both part of the same autochan group, if the chanspy was on 2, it would try to go and grab 1 before it could be destroyed. From there it would go into the spying loop and never come out.
This patch fixed that behavior, but displays the new warning that can be seen in the diff.
I'm going to go ahead and note that this is probably a silly way to use chanspy in the first place since that seems like something better suited for extenspy... but there is some potential for this patch to fix other bugs, so... eh, might be worth doing.</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-19493">ASTERISK-19493</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_chanspy.c <span style="color: grey">(358857)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1819/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>