Re: [asterisk-dev] [Code Review] 3279: Iterate through logger.conf [general] section
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3279/#review11029 --- So, what is needed to get this into 1.8, 11 as it stands now? I'll be trying to use the new config framework and see if I can get it going. - Paul Belanger On Feb. 28, 2014, 4:34 p.m., Paul Belanger wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3279/ --- (Updated Feb. 28, 2014, 4:34 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 trunk/CHANGES 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
Re: [asterisk-dev] [Code Review] 3279: Iterate through logger.conf [general] section
On March 3, 2014, 12:47 p.m., Paul Belanger wrote: So, what is needed to get this into 1.8, 11 as it stands now? I'll be trying to use the new config framework and see if I can get it going. Improvements that do not fix bugs are generally not made in the existing release branches - particularly for LTS releases. 12 had a very different policy from the very beginning, which was communicated ad nauseum. Sometimes, in extremely rare cases, improvements have been allowed in the existing release branches. Generally, this has occurred when there is little to no risk of it affecting existing systems. This patch actually is a behavioural change. If someone had two identical configuration parameters in logger.conf, it changes the preference from the first parameter to the last. While that may be preferable, it does introduce the possibility of an existing system no longer working mid-stream in an LTS release. Barring a very convincing argument about how this cannot impact an existing system, and how this improvement will generally make a large number of people's lives better, I don't think this should be a candidate for inclusion in those branches. - Matt --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3279/#review11029 --- On Feb. 28, 2014, 10:34 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, 10:34 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 trunk/CHANGES 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
Re: [asterisk-dev] [Code Review] 3279: Iterate through logger.conf [general] section
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3279/#review10991 --- 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? - Matt Jordan 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 -- _ -- 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
Re: [asterisk-dev] [Code Review] 3279: Iterate through logger.conf [general] section
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? 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 - Leif --- 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
Re: [asterisk-dev] [Code Review] 3279: Iterate through logger.conf [general] section
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 +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. - wdoekes --- 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
Re: [asterisk-dev] [Code Review] 3279: Iterate through logger.conf [general] section
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.
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?
- 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
--
_
-- 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
Re: [asterisk-dev] [Code Review] 3279: Iterate through logger.conf [general] section
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?
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.
- Russell
---
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
Re: [asterisk-dev] [Code Review] 3279: Iterate through logger.conf [general] section
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
Re: [asterisk-dev] [Code Review] 3279: Iterate through logger.conf [general] section
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
--
_
-- 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
Re: [asterisk-dev] [Code Review] 3279: Iterate through logger.conf [general] section
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3279/ --- (Updated Feb. 28, 2014, 4:05 p.m.) Review request for Asterisk Developers. Changes --- v2: Updated CHANGES 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 (updated) - trunk/main/logger.c 409111 trunk/CHANGES 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
Re: [asterisk-dev] [Code Review] 3279: Iterate through logger.conf [general] section
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3279/ --- (Updated Feb. 28, 2014, 4:34 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 (updated) - trunk/main/logger.c 409111 trunk/CHANGES 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
[asterisk-dev] [Code Review] 3279: Iterate through logger.conf [general] section
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3279/ --- 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
