<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/3409/">https://reviewboard.asterisk.org/r/3409/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 30th, 2014, 6:44 p.m. UTC, <b>Matt Jordan</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;">There are a large number of coding guideline violations in this patch. Please review the Coding Guidelines on the Asterisk wiki [1] and re-submit this patch.
Please also clean up any extraneous white space in your patch (which will show up as red blobs).
The comment explaining the added conditional could use some clean up for clarity, grammar, and spelling.
Finally, I'm not sure how this actually solves ASTERISK-16115. One of the major problems in that issue is that a Queue member in two queues will sometimes receive multiple calls. All this does is check the state of the member status more times:
(original): !(status == AST_DEVICE_NOT_INUSE || status == AST_DEVICE_UNKNOWN)
additional checks: state == AST_DEVICE_INUSE || state == AST_DEVICE_BUSY || state == AST_DEVICE_RINGING || state == AST_DEVICE_RINGINUSE || state == AST_DEVICE_ONHOLD
This is basically just checking the same state twice. You may be bypassing a race condition by checking the state additional times, but I'm not sure how this actually solves the problem in the issue, or how it solves it for multiple queues.
</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;">At first i opened separate bug, where in my case the member does not exist in few queues - https://issues.asterisk.org/jira/browse/ASTERISK-23378 . The Bug was closed and marked as duplicated.
I'm not the best person to explain the case, so sorry for misunderstanding, but again the problem in my case was that if member and it's extension is configure in realtime, while he is in call and there is another incoming call, by running "sip reload" i can reproduce the case when he get's state as "NOT_INUSE", that's why getting call waiting, and there are few customers complaining about it, both on 1.8 and 11.6 that it happens at least few times a day, while after patch all complaining is gone. That's why originally in patch i use i check channels only if extension is realtime.
If you think it's not an issue, we can close the request and i'll keep using my patch by myself only, but i think it would be unfair to use FOSS and not to give back, so i wanted to share it with others. So please tell me if i need to reapply it according to the Coding Guidelines or it doesn't matter anymore.
</pre>
<br />
<p>- Shlomi</p>
<br />
<p>On March 30th, 2014, 2:59 p.m. UTC, Shlomi Gutman 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.</div>
<div>By Shlomi Gutman.</div>
<p style="color: grey;"><i>Updated March 30, 2014, 2: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-16115">ASTERISK-16115</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;">In some cases when member in talk (IN_USE) and you run "sip reload" or peer has connectivity problems (reachable->unreachable/lagged->reachable) the status of peer is set to NOT_INUSE, while he is still talking.
The patch adds function that would check if member has any active channel. And would consider him IN_USE if does and if not, would do nothing, as you can't detect what is the state of member if he has no channel.
Originally patch was used on 1.8.23.0 (can apply it to reviewboard as well) and 11.6-cert1. As well as in my case all peers, queues, memebrs are realtime. But as David Brillert reported in bug, he is not using realtime peers, so it's used for all peers.
The patch works, the question is if it's the best solution and it would not introduce any regressions.</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;">Used in production on 1.8 and 11 and customers having problems approved that problem was resolved.</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/11/apps/app_queue.c <span style="color: grey">(411575)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3409/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>