<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/1919/">https://reviewboard.asterisk.org/r/1919/</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 17th, 2012, 12:24 p.m., <b>Mark Michelson</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/1919/diff/1/?file=27913#file27913line7209" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/apps/app_queue.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 void reload_single_member(const char *memberdata, struct call_queue *q)</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">7207</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_log</span><span class="p">(</span><span class="n">LOG_ERROR</span><span class="p">,</span> <span class="s">"Member %s has an invalid ringinuse value. Using %s ringinuse value.</span><span class="se">\n</span><span class="s">"</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">7208</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">membername</span><span class="p">,</span> <span class="n">q</span><span class="o">-></span><span class="n">name</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">7209</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ringinuse</span> <span class="o">=</span> <span class="n">q</span><span class="o">-></span><span class="n">ringinuse</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">7210</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">7211</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span> <span class="k">else</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">7212</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ringinuse</span> <span class="o">=</span> <span class="n">q</span><span class="o">-></span><span class="n">ringinuse</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">7213</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <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;">You've got a bit of a problem here. For statically defined queues, it's possible that we will begin visiting member config settings before the ringinuse option has been set for the queue. This may result in an inaccurate setting for members of the queue.
Note that this problem does not exist for realtime queues and queue members because all queue settings are applied first and then all queue member settings are applied. It works itself naturally this way since queues and queue members are separate tables.</pre>
</blockquote>
<p>On May 17th, 2012, 1 p.m., <b>jrose</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 suppose that might be considered a problem, but it's hardly a big one and it could even be seen as useful for making the setup of members easier. For instance,
[queuename]
;queues that should ringinuse
ringinuse=yes
member => ...
member => ...
member => ...
...
;queues that should not ringinuse
ringinuse=no
member => ...
member => ...
member => ...
...
which saves the person configuring the need to have 5 or 6 commas on every single member which has that value set.
As long as this is documented, I think most people would actually prefer it to work this way. 'Fixing' this would be moderately annoying since it would basically necessitate making the ringinuse of a member require a seperate variable indicating that it has been deliberately set and if not, then using the queue ringinuse. If this is still not seen as ideal though, I'll make the necessary changes.</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;">Structuring configuration files in a way that the order of options affects behavior is just, in general, a bad idea (tm). There are a few reasons why this is not a good idea.
1. This can lead to subtle (or not-so-subtle) misbehavior when someone upgrades their installation to Asterisk 11 if they have configured their members before their other queue options in queues.conf.
2. For people who use static realtime for their configuration, there is no guarantee for the order in which the generated ast_config structure will sort the options.
3. Under Terry's new configuration architecture, it will be much more difficult to enforce order of options in a configuration file.
I think based on your last paragraph, you may have misunderstood my concern here. I absolutely think it's a good idea for members to inherit the value of the queue's ringinuse option. That is by far the easiest way to go here. The problem is the way that reload_single_queue() processes options. Options are processed variable-by-variable in the ast_config structure, meaning that if you come across a member variable before a ringinuse variable, then the member will not properly inherit the ringinuse option because it has not been visited yet. So you have two potential options here:
1. Specifically seek out the ringinuse option first via ast_variable_retrieve() so that it is guaranteed to be set properly before member variables are processed.
2. Defer member processing until all other queue options have first been processed.
Personally, I find option 2 more forward-thinking and would prefer it. There is a for loop in reload_single_queue() at the moment that iterates over the variables in the config. You can alter the code to run the loop twice. One time is used to run through all non-member options and the second time is to run through the member options.</pre>
<br />
<p>- Mark</p>
<br />
<p>On May 17th, 2012, 1:02 p.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, irroot, and Matt Jordan.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated May 17, 2012, 1:02 p.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 bug was simply the catalyst for this patch. There was another patch that actually resolved this bug...
Here's the summary:
First, only the member setting for ringinuse is checked when determining whether to call the busy member or not.
The default for this value is now determined by the queue's ringinuse option rather than being standalone.
additions
Adds the ability to specify member ringinuse value as an argument
Adds 'queue set ringinuse' CLI command for setting ringinuse for a member on the fly
Adds AMI command QueueRingInUse for setting ringinuse for a member on the fly via AMI
changes
'ignorebusy' is now called 'ringinuse'
Legacy support for old dialplans/scripts/etc will still be matched via the QUEUE_MEMBER function argument 'ignorebusy', but 'ringinuse' works as well and is the documented argument name.
Legacy support for realtime 'ignorebusy' is in place, but at startup of the module if no instances of its use are present in the database or if a use of the 'ringinuse' field is detected, that will be used exclusively instead.</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 tested a number of configurations of ringinuse for queues and members both realtime and otherwise to make sure the setting went in correctly under each case.
There is an odd quirk though for realtime in that if all the values of a field for every entry are NULL, then it will be as though that field doesn't exist at all, so if ignorebusy is present without any deliberately entered values, ringinuse will still become the defacto field name in use. I worked at it for a while but couldn't find a workaround that didn't involve having to change every single realtime database driver. It's not that big of a deal since if a user isn't actually using that field at all, it's unlikely they ever intended to.</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-19536">ASTERISK-19536</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>/trunk/CHANGES <span style="color: grey">(366295)</span></li>
<li>/trunk/apps/app_queue.c <span style="color: grey">(366295)</span></li>
<li>/trunk/configs/queues.conf.sample <span style="color: grey">(366295)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1919/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>