> On Feb. 28, 2014, 12:51 p.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.
@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. - Paul ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3279/#review10991 ----------------------------------------------------------- On Feb. 28, 2014, 1:44 a.m., Paul Belanger wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3279/ > ----------------------------------------------------------- > > (Updated Feb. 28, 2014, 1:44 a.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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
