<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/3279/">https://reviewboard.asterisk.org/r/3279/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 28th, 2014, 12:51 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;">I'm not sure I understand the need for this patch.
Setting a configuration option twice - when that option doesn't support being set multiple times - would generally have undefined behaviour. Your patch changes it so that Asterisk reads the last defined value, as opposed to the first. How is that better?</pre>
</blockquote>
<p>On February 28th, 2014, 1:52 p.m. UTC, <b>Leif Madsen</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;">It's better when you want to deploy Asterisk in a DevOps environment. What happens is you define the "default" behavior in the main file that gets deployed. From there you then override that behavior in an included file which is defined and built via DevOps (usually through a template of some sort that contains information for the particular machine you're deploying).
The example is not a good one, because obviously you would never deploy the file in the example provided.
; logger.conf
[general]
queue_log=no
#include logger.conf.d/logger.conf.local
; logger.conf.d/logger.conf.local
[general]
queue_log=yes ; we've deployed a queue server, so enable queue logging
Primary example of it in use available at https://github.com/kickstandproject/kickstandproject-asterisk/tree/master/templates/etc/asterisk</pre>
</blockquote>
<p>On February 28th, 2014, 2:34 p.m. UTC, <b>wdoekes</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;">+1 on iterating over the configs options. Most modules do, some don't. Consistency is nice.
Please do add a note to the upgrade file though. Perhaps someone has already worked around this by placing the #include logger.conf.d statement at the top of the file.</pre>
</blockquote>
<p>On February 28th, 2014, 2:38 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;">I can see how this might be better for that deployment scenario.
I compared this approach against the Config Framework (config_options.c). It uses an ast_variable_browse as well (which is not surprising, since it has to parse through a list of key/value pairs), so this does take this into line with the recommended (tm) way of doing things:
int aco_process_category_options(struct aco_type *type, struct ast_config *cfg, const char *cat, void *obj)
{
struct ast_variable *var;
for (var = ast_variable_browse(cfg, cat); var; var = var->next) {
if (aco_process_var(type, cat, var, obj)) {
return -1;
}
}
return 0;
}
That being said, why not just bite the bullet and use the configuration framework for logger.conf?</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;">Patch looks fine to me. It's an improvement over what's there, for sure. Using new-style config handling sounds nice, but this is at least a step in a better direction.</pre>
<br />
<p>- Russell</p>
<br />
<p>On February 28th, 2014, 1:44 a.m. UTC, Paul Belanger 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 Paul Belanger.</div>
<p style="color: grey;"><i>Updated Feb. 28, 2014, 1:44 a.m.</i></p>
<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;">This patch allows you to override the [general] section of logger.conf, making it the same functionality as the [logfiles] sections.</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;">local development. Setup
[general]
queue_log = no
queue_log = yes
Queue logfiles were created.
</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>trunk/main/logger.c <span style="color: grey">(409111)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3279/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>