[asterisk-dev] [Code Review] 3279: Iterate through logger.conf [general] section

[Matt Jordan]

> On Feb. 28, 2014, 6:51 a.m., Matt Jordan wrote:
> > 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?
> 
> Leif Madsen wrote:
>     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
> 
> wdoekes wrote:
>     +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.
> 
> Matt Jordan wrote:
>     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?
> 
> Russell Bryant wrote:
>     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.
> 
> Paul Belanger wrote:
>     @Matt: Reason for not using the new configuration options was because we are back porting this patch into 1.8, which we've tested with.  If committed, I can try my hand at using the new config_options framework and try upgrading the code.

Well, as an improvement to Asterisk, it really should be done using the Configuration Framework. You're more than welcome to use this patch in your 1.8 code (obviously :-) ) - but the configuration framework was written to try and enforce some sanity on the configuration parsing in Asterisk. It has a lot of other benefits as well - enforced documentation of parameters, CLI/wiki integration, etc.

If you're going to go ahead and improve Asterisk, it'd be nice if it was done using the recommended frameworks to do so.


- Matt


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3279/#review10991
-----------------------------------------------------------


On Feb. 27, 2014, 7:44 p.m., Paul Belanger wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3279/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2014, 7:44 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch allows you to override the [general] section of logger.conf, making it the same functionality as the [logfiles] sections.
> 
> 
> Diffs
> -----
> 
>   trunk/main/logger.c 409111 
> 
> Diff: https://reviewboard.asterisk.org/r/3279/diff/
> 
> 
> Testing
> -------
> 
> local development. Setup
> 
> [general]
> queue_log = no
> queue_log = yes
> 
> Queue logfiles were created.
> 
> 
> Thanks,
> 
> Paul Belanger
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140228/ca28dfa1/attachment.html>