<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/1234/">https://reviewboard.asterisk.org/r/1234/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 27th, 2011, 5:25 p.m., <b>Russell Bryant</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 think there may be a race condition here. Can two threads both determine that a channel can be picked up using ast_can_pickup() and then proceed to attempt to attempt a pickup? I'm not sure if there's a good way to fix it. Maybe the pickup function just also needs to check for this datastore and bail if it is already there.
I'm also wondering if there could be bad side effects of leaving the datastore on the channel. It's going to hang around and keep getting moved if transfers and such happen. Will there _ever_ be a case when it's valid to do a pickup on the same ast_channel twice? I can't think of one but leaving that data around still doesn't feel right.</pre>
</blockquote>
<p>On May 27th, 2011, 5:44 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;">Two threads cannot determine that a channel can be picked up at the same time by ast_can_pickup(). The ast_can_pickup() is a common helper function. The callers have the channel locked as a precondition and will not release the lock before ast_do_pickup() is called. Other threads would try to pickup the same call without the lock.
The current code does not leave the datastore on the channel. I thought I would need to leave it to prevent other threads from stealing the just picked up channel away from us. The code is conditionaled out. I left it there because I did not want to recreate the comment block if I had to put it back in. I cannot think of a way a call can be picked up twice either since once a call is picked up the channel state is UP and it cannot/should-not go back to a ringing state. (It might possibly go to a DOWN state on hangup. It would not be a good thing to pickup again then anyway.) Also that datastore is not inheritable.
</pre>
</blockquote>
<p>On May 29th, 2011, 4:39 p.m., <b>Alec Davis</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;">Just putting this thought out there, which is not a race between 2 directed pickups.
What if the ringing extension is about to be answered by lifting the receiver or to go to voicemail, and someelse tries to do directed pickup of the same ringing extension.
Could, at the right time, the directed pickup steal the conventionally answered call?</pre>
</blockquote>
<p>On May 29th, 2011, 7:18 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;">If the ringing extension is about to goto voice mail, I don't think what happens would make any difference if the call were normally answered or picked up. It would either be answered or goto voice mail.
I think it would be hard for the user to be able to tell if the call was actually stolen by the pickup.</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;">Oh ok, I see now. I was the lock and unlock around the code calling ast_can_pickup() so I thought there could be a problem. Now I see that the channel lock isn't released in the case that the channel was chosen for pickup.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 27th, 2011, 5:25 p.m., <b>Russell Bryant</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/1234/diff/1/?file=16760#file16760line129" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/include/asterisk/features.h</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; ">int ast_parking_ext_valid(const char *exten_str, struct ast_channel *chan, const char *context);</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">129</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> * \return TRUE if channel can be picked up.</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 know this is pedantic, but can you be more explicit about the return value and say non-zero instead of true here since the return type is int?</pre>
</blockquote>
<p>On May 27th, 2011, 5:42 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 have used this type of return comment is a bunch of places and it is a typical C idiom.</pre>
</blockquote>
<p>On May 29th, 2011, 7:08 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;">Also I want the user to be thinking of boolean usage and not the actual value.</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;">I was just thinking ...
\retval 0 channel can not be picked up
\retval non-zero channel can be picked up
it's not a big deal. What you have makes the point.</pre>
<br />
<p>- Russell</p>
<br />
<p>On May 26th, 2011, 6:31 p.m., rmudgett 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, Alec Davis and irroot.</div>
<div>By rmudgett.</div>
<p style="color: grey;"><i>Updated 2011-05-26 18:31:19</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;">Fix potential deadlock in ast_do_pickup() with channels chan and target lock and channel container lock.
This should eliminate the deadlock avoidance code in https://reviewboard.asterisk.org/r/1118/ for issue 18830.
* Moved can_pickup() to ast_can_pickup() in features.c.</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 a simple call pickup to make sure that it at least still works.
Needs competing call pickup testing.</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=18654">18654</a>,
<a href="https://issues.asterisk.org/view.php?id=18830">18830</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/include/asterisk/features.h <span style="color: grey">(321209)</span></li>
<li>/branches/1.8/main/features.c <span style="color: grey">(321209)</span></li>
<li>/branches/1.8/apps/app_directed_pickup.c <span style="color: grey">(321209)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1234/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>