Re: [aur-dev][PATCH] config: allow reading both the proto file and the modified config
I think you're right. I looked one more time and found guard there: if (!isset($AUR_CONFIG)) Thanks for clarification. Nodiv. On Mon, Apr 16, 2018 at 1:41 AM, Eli Schwartz wrote: > On 04/12/2018 02:22 PM, Nodiv Byzero wrote: >> The point is that there are few places where config_load() gets called >> in the sequential order (the same request) and every time it loads and >> parses data. > > Are these also places where if (!isset($AUR_CONFIG)) fails to actually > fulfill its sole reason to exist? > > Not that it matters... if for some reason we are loading the config very > fast and scribbling over each other, because $AUR_CONFIG is not yet set > when we begin the second config_load() execution, checking if $config is > already loaded won't help -- it's a local variable. > > So, question: can you show me code where $config would *not* be `false`? > > Because I think this proposition would be either a no-op or a reason to > file a severely major upstream php bug complaining that the variables > are leaking all over the floor. > > ... > > That being said, yes, there is a race condition where you call > config_load() twice, and they both set $AUR_CONFIG. > > Elevating $config and $default_config to globals, *then* using them as > additional file access caches to micro-optimize one or two disk ops, > seems wasteful to me. It's not like this is being run in a tight loop. > > If any effort should be spent to fix this, rather than introducing > painful, non-obvious code, we should introduce some sort of proper cache. > > -- > Eli Schwartz > Bug Wrangler and Trusted User >
[aur-dev][PATCH] config: allow reading both the proto file and the modified config
On 04/12/2018 02:22 PM, Nodiv Byzero wrote: > The point is that there are few places where config_load() gets called > in the sequential order (the same request) and every time it loads and > parses data. Are these also places where if (!isset($AUR_CONFIG)) fails to actually fulfill its sole reason to exist? Not that it matters... if for some reason we are loading the config very fast and scribbling over each other, because $AUR_CONFIG is not yet set when we begin the second config_load() execution, checking if $config is already loaded won't help -- it's a local variable. So, question: can you show me code where $config would *not* be `false`? Because I think this proposition would be either a no-op or a reason to file a severely major upstream php bug complaining that the variables are leaking all over the floor. ... That being said, yes, there is a race condition where you call config_load() twice, and they both set $AUR_CONFIG. Elevating $config and $default_config to globals, *then* using them as additional file access caches to micro-optimize one or two disk ops, seems wasteful to me. It's not like this is being run in a tight loop. If any effort should be spent to fix this, rather than introducing painful, non-obvious code, we should introduce some sort of proper cache. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [aur-dev][PATCH] config: allow reading both the proto file and the modified config
On 04/15/2018 06:11 AM, Lukas Fleischer wrote: > Maybe even better: make AUR_CONFIG default to /etc/aurweb/config and > AUR_CONFIG_DEFAULTS default to the value of AUR_CONFIG with an > additional ".defaults" suffix. Great minds think alike. :) -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [aur-dev][PATCH] config: allow reading both the proto file and the modified config
On Sat, 14 Apr 2018 at 16:04:58, Lukas Fleischer wrote: > On Thu, 12 Apr 2018 at 19:44:15, Eli Schwartz wrote: > > This change allows aurweb configuration to be done via either: > > - copying config.proto to config and modifying values > > - creating a new config only containing modified values, next to a > > config.proto containing unmodified values > > > > The motivation for this change is to enable ansible configuration by > > storing only changed values, and deferring to config.proto otherwise. > > > > If a config.proto file does not exist next to /etc/aurweb/config or > > $AUR_CONFIG, it is ignored and *all* values are expected to live in the > > modified config file. > > > > Signed-off-by: Eli Schwartz > > --- > > aurweb/config.py | 4 > > web/lib/confparser.inc.php | 8 +++- > > 2 files changed, 11 insertions(+), 1 deletion(-) > > [...] > > Thanks, I like the idea. However, I think it would be better to make > both the path to the defaults file and the path to the configuration > file itself configurable (i.e. add a new environment variable > AUR_CONFIG_DEFAULTS). By default, /etc/aurweb/config.defaults and > /etc/aurweb/config could be used. Maybe even better: make AUR_CONFIG default to /etc/aurweb/config and AUR_CONFIG_DEFAULTS default to the value of AUR_CONFIG with an additional ".defaults" suffix. Then, if no environment variables are set, aurweb would look for default values in /etc/aurweb/config.defaults and for additional configuration in /etc/aurweb/config. If only AUR_CONFIG is set (say, to /some/path), aurweb looks for default values under /some/path.defaults and for additional configuration under /some/path. And if you really want to store the defaults somewhere else, you can set AUR_CONFIG_DEFAULTS. Best regards, Lukas Fleischer
Re: [aur-dev][PATCH] config: allow reading both the proto file and the modified config
On Thu, 12 Apr 2018 at 19:44:15, Eli Schwartz wrote: > This change allows aurweb configuration to be done via either: > - copying config.proto to config and modifying values > - creating a new config only containing modified values, next to a > config.proto containing unmodified values > > The motivation for this change is to enable ansible configuration by > storing only changed values, and deferring to config.proto otherwise. > > If a config.proto file does not exist next to /etc/aurweb/config or > $AUR_CONFIG, it is ignored and *all* values are expected to live in the > modified config file. > > Signed-off-by: Eli Schwartz > --- > aurweb/config.py | 4 > web/lib/confparser.inc.php | 8 +++- > 2 files changed, 11 insertions(+), 1 deletion(-) > [...] Thanks, I like the idea. However, I think it would be better to make both the path to the defaults file and the path to the configuration file itself configurable (i.e. add a new environment variable AUR_CONFIG_DEFAULTS). By default, /etc/aurweb/config.defaults and /etc/aurweb/config could be used. Best regards, Lukas
Re: [aur-dev][PATCH] config: allow reading both the proto file and the modified config
The point is that there are few places where config_load() gets called
in the sequential order (the same request) and every time it loads and
parses data.
On Thu, Apr 12, 2018 at 11:05 AM, Eli Schwartz wrote:
> On 04/12/2018 01:51 PM, Nodiv Byzero wrote:
>> What do you think about adding one more line to check if $config is already
>> set?
>> Something like:
>> if (!isset($config)) {
>> ... do the parse_ini_file
>> }
>>
>> Just to reduce disk operations.
>
> What is the point? We only check if $AUR_CONFIG isset() because it is
> explicitly global, and the function is here to set it if needed or else
> do nothing.
>
> $config is locally scoped, and its only purpose is exactly where it is
> being used. It should be utterly impossible for it to be set beforehand.
>
> ...
>
> On an unrelated note, Bluewind suggested renaming the config.proto to
> config.defaults, so if Lukas agrees with this patch I will send in an
> amended version.
>
> --
> Eli Schwartz
> Bug Wrangler and Trusted User
>
Re: [aur-dev][PATCH] config: allow reading both the proto file and the modified config
On 04/12/2018 01:51 PM, Nodiv Byzero wrote:
> What do you think about adding one more line to check if $config is already
> set?
> Something like:
> if (!isset($config)) {
> ... do the parse_ini_file
> }
>
> Just to reduce disk operations.
What is the point? We only check if $AUR_CONFIG isset() because it is
explicitly global, and the function is here to set it if needed or else
do nothing.
$config is locally scoped, and its only purpose is exactly where it is
being used. It should be utterly impossible for it to be set beforehand.
...
On an unrelated note, Bluewind suggested renaming the config.proto to
config.defaults, so if Lukas agrees with this patch I will send in an
amended version.
--
Eli Schwartz
Bug Wrangler and Trusted User
signature.asc
Description: OpenPGP digital signature
Re: [aur-dev][PATCH] config: allow reading both the proto file and the modified config
What do you think about adding one more line to check if $config is already set?
Something like:
if (!isset($config)) {
... do the parse_ini_file
}
Just to reduce disk operations.
On Thu, Apr 12, 2018 at 10:44 AM, Eli Schwartz wrote:
> This change allows aurweb configuration to be done via either:
> - copying config.proto to config and modifying values
> - creating a new config only containing modified values, next to a
> config.proto containing unmodified values
>
> The motivation for this change is to enable ansible configuration by
> storing only changed values, and deferring to config.proto otherwise.
>
> If a config.proto file does not exist next to /etc/aurweb/config or
> $AUR_CONFIG, it is ignored and *all* values are expected to live in the
> modified config file.
>
> Signed-off-by: Eli Schwartz
> ---
> aurweb/config.py | 4
> web/lib/confparser.inc.php | 8 +++-
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/aurweb/config.py b/aurweb/config.py
> index a52d942..c333c43 100644
> --- a/aurweb/config.py
> +++ b/aurweb/config.py
> @@ -13,6 +13,10 @@ def _get_parser():
> path = os.environ.get('AUR_CONFIG')
> else:
> path = "/etc/aurweb/config"
> +
> +if os.path.isfile(path + '.proto'):
> +with open(path + '.proto') as f:
> +_parser.read_file(f)
> _parser.read(path)
>
> return _parser
> diff --git a/web/lib/confparser.inc.php b/web/lib/confparser.inc.php
> index 499481d..2ed0108 100644
> --- a/web/lib/confparser.inc.php
> +++ b/web/lib/confparser.inc.php
> @@ -8,11 +8,17 @@ function config_load() {
> if (!$path) {
> $path = "/etc/aurweb/config";
> }
> + if (file_exists($path . ".proto")) {
> + $defaults = parse_ini_file($path . ".proto", true,
> INI_SCANNER_RAW);
> + } else {
> + $defaults = [];
> + }
> if (file_exists($path)) {
> - $AUR_CONFIG = parse_ini_file($path, true,
> INI_SCANNER_RAW);
> + $config = parse_ini_file($path, true,
> INI_SCANNER_RAW);
> } else {
> die("aurweb config file not found");
> }
> + $AUR_CONFIG = array_replace_recursive($defaults, $config)
> }
> }
>
> --
> 2.17.0
[aur-dev][PATCH] config: allow reading both the proto file and the modified config
This change allows aurweb configuration to be done via either:
- copying config.proto to config and modifying values
- creating a new config only containing modified values, next to a
config.proto containing unmodified values
The motivation for this change is to enable ansible configuration by
storing only changed values, and deferring to config.proto otherwise.
If a config.proto file does not exist next to /etc/aurweb/config or
$AUR_CONFIG, it is ignored and *all* values are expected to live in the
modified config file.
Signed-off-by: Eli Schwartz
---
aurweb/config.py | 4
web/lib/confparser.inc.php | 8 +++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/aurweb/config.py b/aurweb/config.py
index a52d942..c333c43 100644
--- a/aurweb/config.py
+++ b/aurweb/config.py
@@ -13,6 +13,10 @@ def _get_parser():
path = os.environ.get('AUR_CONFIG')
else:
path = "/etc/aurweb/config"
+
+if os.path.isfile(path + '.proto'):
+with open(path + '.proto') as f:
+_parser.read_file(f)
_parser.read(path)
return _parser
diff --git a/web/lib/confparser.inc.php b/web/lib/confparser.inc.php
index 499481d..2ed0108 100644
--- a/web/lib/confparser.inc.php
+++ b/web/lib/confparser.inc.php
@@ -8,11 +8,17 @@ function config_load() {
if (!$path) {
$path = "/etc/aurweb/config";
}
+ if (file_exists($path . ".proto")) {
+ $defaults = parse_ini_file($path . ".proto", true,
INI_SCANNER_RAW);
+ } else {
+ $defaults = [];
+ }
if (file_exists($path)) {
- $AUR_CONFIG = parse_ini_file($path, true,
INI_SCANNER_RAW);
+ $config = parse_ini_file($path, true, INI_SCANNER_RAW);
} else {
die("aurweb config file not found");
}
+ $AUR_CONFIG = array_replace_recursive($defaults, $config)
}
}
--
2.17.0
