From d2183ab5d1876d9f9f10ebbebfd672cc408fba6c Mon Sep 17 00:00:00 2001 From: Michel Jouvin Date: Sun, 8 May 2016 23:02:23 +0200 Subject: [PATCH] RuleBasedEditor: additional comment cleanups and clarifications - Also includes some code reformatting --- src/main/perl/RuleBasedEditor.pm | 118 ++++++++++++++----------------- 1 file changed, 53 insertions(+), 65 deletions(-) diff --git a/src/main/perl/RuleBasedEditor.pm b/src/main/perl/RuleBasedEditor.pm index 3f434c7c..45e9f069 100644 --- a/src/main/perl/RuleBasedEditor.pm +++ b/src/main/perl/RuleBasedEditor.pm @@ -70,34 +70,9 @@ hash). =item line_fmt -Defines the format used to represent the key/value pair. The following formats are -supported (see LINE_FORMAT_xxx constants below): - -=over - -=item * - -A SH shell environment variable definition (export key=val). - -=item * - -A SH shell variable definition (key=val). - -=item * - -A 'keyword value' line, as used by Apache config files (similar to Config::General). - -=item * - -A 'setenv keyword=value' line, as used by Xrootd config files mainly. It doesn't work in a CSH shell script (C<=> present). - -=item * - -A 'set keyword=value' line, as used in a a CSH shell script to define a shell variable. - -=back - -Inline comments are not supported in 'keyword value' family of formats. +Defines the format used to represent the keyword/value pair. Several format are supported covering +the most usual ones (SH shell script, Apache, ...). For the exact list, see the definition of +LINE_FORMAT_xxx constants and the associated documentation below. =item value_fmt @@ -184,6 +159,8 @@ LINE_FORMAT_KEY_VAL_SET: set key=val (CSH shell variable) =back +Inline comments are not supported for the LINE_FORMAT_KEY_VAL_xxx formats. + =cut use enum qw( @@ -253,7 +230,7 @@ LINE_VALUE_OPT_SINGLE: each value must be a separate instance of the keyword (mu =item -LINE_VALUE_OPT_UNIQUE: each values are concataneted as a space-separated string +LINE_VALUE_OPT_UNIQUE: each values are concatenated as a space-separated string =item @@ -303,6 +280,14 @@ our %EXPORT_TAGS; push @EXPORT_OK, @RULE_CONSTANTS; $EXPORT_TAGS{rule_constants} = \@RULE_CONSTANTS; +=pod + +$FILE_INTRO_xxx: constants defining the expected header lines in the configuration file + +=cut + +Readonly my $FILE_INTRO_PATTERN => "# This file is managed by Quattor"; +Readonly my $FILE_INTRO_TXT => "# This file is managed by Quattor - DO NOT EDIT lines generated by Quattor"; # Backup file extension Readonly my $BACKUP_FILE_EXT => ".old"; @@ -318,8 +303,8 @@ Readonly my $BACKUP_FILE_EXT => ".old"; Update configuration file contents, applying configuration rules. Arguments : - config_rules: config rules corresponding to the file to build - config_options: configuration parameters used to build actual configuration + config_rules: a hashref containing config rules corresponding to the file to build + config_options: a hashref for configuration parameters used to build actual configuration options: a hashref defining options to modify the behaviour of this function Supported entries for options hash: @@ -329,7 +314,7 @@ Supported entries for options hash: Return value sucess: 1 - argument error: undef + argument error or error duing rule processing: undef =cut @@ -354,12 +339,10 @@ sub updateFile $self->seek_begin(); # Check that config file has an appropriate header - Readonly my $INTRO_PATTERN => "# This file is managed by Quattor"; - my $intro = "# This file is managed by Quattor - DO NOT EDIT lines generated by Quattor"; $self->add_or_replace_lines( - qr/^$INTRO_PATTERN/, - qr/^$intro$/, - $intro . "\n#\n", + qr/^$FILE_INTRO_PATTERN/, + qr/^$FILE_INTRO_TXT$/, + $FILE_INTRO_TXT . "\n#\n", BEGINNING_OF_FILE, ); @@ -433,7 +416,7 @@ sub _formatAttributeValue # LINE_VALUE_INSTANCE_PARAMS is a value format specific to XrootD (http://xrootd.org). # The value is a hash containing 3 keys that are used to construct a command option line. $formatted_value = ''; # Don't return if no matching attributes is found - # Instance parameters are described in a nlist + # Instance parameters are described in a hash $formatted_value .= " -l $attr_value->{logFile}" if $attr_value->{logFile}; $formatted_value .= " -c $attr_value->{configFile}" if $attr_value->{configFile}; $formatted_value .= " -k $attr_value->{logKeep}" if $attr_value->{logKeep}; @@ -663,6 +646,7 @@ sub _commentConfigLine # The code used is a customized version of FileEditor::replace() that lacks support for backreferences # in the replacement value (here we want to rewrite the same line commented out but we don't know the # current line contents, only a regexp matching it). + # FIXME: should be updated when https://github.com/quattor/CAF/issues/125 is fixed. my @lns; my $line_count = 0; $self->seek_begin(); @@ -914,7 +898,8 @@ Apply configuration rules. This method is the real workhorse of the rule-based e Arguments : config_rules: config rules corresponding to the file to build - config_options: configuration parameters used to build actual configuration + config_options: configuration parameters used to build actual configuration. Note that keys in the + config_options hash are interpreted as escaped (harmless if they are not). parser_options: a hash setting options to modify the behaviour of this function Supported entries for options hash: @@ -974,7 +959,7 @@ sub _apply_rules $rule_id++; # Initialize parser_options for this rule according the default for this file - my $rule_parsing_options = {%{$parser_options}}; + my $rule_parsing_options = {%$parser_options}; # Check if the keyword is prefixed by: # - a '-': in this case the corresponding line must be unconditionally @@ -1010,8 +995,10 @@ sub _apply_rules # If the keyword was "negated", remove (comment out) configuration line if present and enabled if ($comment_line) { - *$self->{LOG}->debug(1, "$function_name: keyword '$keyword' negated, removing configuration line"); - $self->_commentConfigLine($keyword, $line_fmt); + *$self->{LOG}->debug(1, "$function_name: keyword '$keyword' negated, removing/commenting configuration line"); + unless ( $self->_commentConfigLine($keyword, $line_fmt) ) { + *$self->{LOG}->error("$function_name: _commentConfigLine() encountered an internal error, lines matching '$keyword' not removed"); + } next; } @@ -1019,20 +1006,24 @@ sub _apply_rules # Parse rule if it is non empty my $rule_info; if ($rule ne '') { - *$self->{LOG} - ->debug(1, "$function_name: processing rule $rule_id (variable=>>>$keyword<<<, rule=>>>$rule<<<, fmt=$line_fmt)"); + *$self->{LOG}->debug(1, "$function_name: processing rule $rule_id (variable=>>>$keyword<<<, rule=>>>$rule<<<, fmt=$line_fmt)"); $rule_info = $self->_parse_rule($rule, $config_options, $rule_parsing_options); next unless $rule_info; *$self->{LOG}->debug(2, "$function_name: information returned by rule parser: " . join(" ", sort(keys(%$rule_info)))); if (exists($rule_info->{error_msg})) { + # FIXME: decide whether an invalid rule is considered an error or just a warning. The latter would + # allow the caller to decide what to do exactly rather than impose an error (meaning a + # potential dependency failure) *$self->{LOG}->error("Error parsing rule >>>$rule<<<: " . $rule_info->{error_msg}); # An invalid rule is just ignored next; } elsif ($rule_info->{remove_matching_lines}) { if ($rule_parsing_options->{remove_if_undef}) { - *$self->{LOG}->debug(1, "$function_name: removing configuration lines for keyword '$keyword'"); - $self->_commentConfigLine($keyword, $line_fmt); + *$self->{LOG}->debug(1, "$function_name: removing/commenting configuration lines for keyword '$keyword'"); + unless ( $self->_commentConfigLine($keyword, $line_fmt) ) { + *$self->{LOG}->error("$function_name: _commentConfigLine() encountered an internal error, lines matching '$keyword' not removed"); + } } else { *$self->{LOG}->debug(1, "$function_name: remove_if_undef not set, ignoring line to remove"); } @@ -1040,7 +1031,7 @@ sub _apply_rules } } - # Build the value to be substitued for each option set specified. + # Build the value to be substituted for each option set specified. # option_set=GLOBAL is a special case indicating a global option instead of an # attribute in a specific option set. my $config_value = ""; @@ -1050,14 +1041,12 @@ sub _apply_rules if ($rule_info->{attribute}) { foreach my $option_set (@{$rule_info->{option_sets}}) { my $attr_value; - *$self->{LOG} - ->debug(1, "$function_name: retrieving '" . $rule_info->{attribute} . "' value in option set $option_set"); + *$self->{LOG}->debug(1, "$function_name: retrieving '" . $rule_info->{attribute} . "' value in option set $option_set"); if ($option_set eq $RULE_OPTION_SET_GLOBAL) { - if (exists($config_options->{$rule_info->{attribute}})) { + if ( $config_options->{$rule_info->{attribute}} ) { $attr_value = $config_options->{$rule_info->{attribute}}; } else { - *$self->{LOG} - ->debug(1, "$function_name: attribute '" . $rule_info->{attribute} . "' not found in global option set"); + *$self->{LOG}->debug(1, "$function_name: attribute '" . $rule_info->{attribute} . "' not found in global option set"); $attribute_present = 0; } } else { @@ -1090,21 +1079,19 @@ sub _apply_rules next; } - # Instance parameters are specific, as this is a nlist of instance - # with the value being a nlist of parameters for the instance. + # Instance parameters are specific, as this is a hash of instances + # with the value being a hash of parameters for the instance. # Also the variable name must be updated to contain the instance name. # One configuration line must be written/updated for each instance. if ($value_fmt == LINE_VALUE_INSTANCE_PARAMS) { foreach my $instance (sort keys %{$attr_value}) { my $params = $attr_value->{$instance}; *$self->{LOG}->debug(1, "$function_name: formatting instance '$instance' parameters ($params)"); - $config_value = - $self->_formatAttributeValue( - $params, - $line_fmt, - $value_fmt, - $value_opt, - ); + $config_value = $self->_formatAttributeValue($params, + $line_fmt, + $value_fmt, + $value_opt, + ); my $config_param = $keyword; my $instance_uc = uc($instance); $config_param =~ s/%%INSTANCE%%/$instance_uc/; @@ -1114,12 +1101,13 @@ sub _apply_rules $config_updated = 1; } elsif ($value_fmt == LINE_VALUE_STRING_HASH) { # With this value format, several lines with the same keyword are generated, - # one for each key/value pair. + # one for each keyword/value pair. foreach my $k (sort keys %$attr_value) { my $v = $attr_value->{$k}; - # Value is made by joining key and value as a string - # Keys may be escaped if they contain characters like '/': unescaping a non-escaped - # string is generally harmless. + # Value is made by joining key and value as a string. + # Keys may be escaped if they contain characters like '/'. This is not a problem if they + # are not as unescaping a non-escaped string is harmless (except for very specific and + # unlikely value matching an escaped character like '_2e'). my $tmp = unescape($k) . " $v"; *$self->{LOG}->debug(1, "$function_name: formatting (string hash) attribute '"