[pve-devel] [PATCH v2 pve-common 3/4] section config: clean up parser logic and semantics

[Max Carrara]

In order to make the parser somewhat more maintainable in the future,
this commit cleans up its logic and makes its control flow easier to
follow.

Furthermore, regexes are assigned to variables and some variables are
renamed in order to make the code more semantically expressive.

Signed-off-by: Max Carrara <m.carrara at proxmox.com>
---
Changes v1 --> v2:
  * reword commit message
  * for errors, use reference of array instead of array directly
  * drop 'x' modifier from $re_kv_pair regex, as it doesn't have any
    benefit

 src/PVE/SectionConfig.pm | 187 ++++++++++++++++++++-------------------
 1 file changed, 97 insertions(+), 90 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index a6cc468..ec074fa 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -1190,25 +1190,26 @@ The error.
 sub parse_config {
     my ($class, $filename, $raw, $allow_unknown) = @_;
 
-    my $pdata = $class->private();
+    if (!defined($raw)) {
+	return {
+	    ids => {},
+	    order => {},
+	    digest => Digest::SHA::sha1_hex(''),
+	};
+    }
+
+    my $re_begins_with_comment = qr/^\s*#/;
+    my $re_kv_pair = qr/^\s+(\S+)(\s+(.*\S))?\s*$/;
 
     my $ids = {};
     my $order = {};
-
-    $raw = '' if !defined($raw);
-
     my $digest = Digest::SHA::sha1_hex($raw);
 
-    my $pri = 1;
+    my $current_section_no = 1;
+    my $line_no = 0;
 
-    my $lineno = 0;
     my @lines = split(/\n/, $raw);
-    my $nextline = sub {
-	while (defined(my $line = shift @lines)) {
-	    $lineno++;
-	    return $line if ($line !~ /^\s*#/);
-	}
-    };
+    my $errors = [];
 
     my $is_array = sub {
 	my ($type, $key) = @_;
@@ -1219,105 +1220,111 @@ sub parse_config {
 	return $schema->{type} eq 'array';
     };
 
-    my $errors = [];
-    while (@lines) {
-	my $line = $nextline->();
+    my $get_next_line = sub {
+	while (scalar(@lines)) {
+	    my $line = shift(@lines);
+	    $line_no++;
+
+	    next if ($line =~ m/$re_begins_with_comment/);
+
+	    return $line;
+	}
+
+	return undef;
+    };
+
+    my $skip_to_next_empty_line = sub {
+	while ($get_next_line->() ne '') {}
+    };
+
+    while (defined(my $line = $get_next_line->())) {
 	next if !$line;
 
-	my $errprefix = "file $filename line $lineno";
+	my $errprefix = "file $filename line $line_no";
 
-	my ($type, $sectionId, $errmsg, $config) = $class->parse_section_header($line);
-	if ($config) {
-	    my $skip = 0;
-	    my $unknown = 0;
+	my ($type, $section_id, $errmsg, $config) = $class->parse_section_header($line);
 
-	    my $plugin;
+	if (!defined($config)) {
+	    warn "$errprefix - ignore config line: $line\n";
+	    next;
+	}
 
-	    if ($errmsg) {
-		$skip = 1;
-		chomp $errmsg;
-		warn "$errprefix (skip section '$sectionId'): $errmsg\n";
-	    } elsif (!$type) {
-		$skip = 1;
-		warn "$errprefix (skip section '$sectionId'): missing type - internal error\n";
-	    } else {
-		if (!($plugin = $pdata->{plugins}->{$type})) {
-		    if ($allow_unknown) {
-			$unknown = 1;
-		    } else {
-			$skip = 1;
-			warn "$errprefix (skip section '$sectionId'): unsupported type '$type'\n";
-		    }
-		}
-	    }
+	if ($errmsg) {
+	    chomp $errmsg;
+	    warn "$errprefix (skip section '$section_id'): $errmsg\n";
+	    $skip_to_next_empty_line->();
+	    next;
+	}
+
+	if (!$type) {
+	    warn "$errprefix (skip section '$section_id'): missing type - internal error\n";
+	    $skip_to_next_empty_line->();
+	    next;
+	}
+
+	my $plugin = eval { $class->lookup($type) };
+	my $is_unknown_type = defined($@) && $@ ne '';
+
+	if ($is_unknown_type && !$allow_unknown) {
+	    warn "$errprefix (skip section '$section_id'): unsupported type '$type'\n";
+	    $skip_to_next_empty_line->();
+	    next;
+	}
 
-	    while ($line = $nextline->()) {
-		next if $skip;
-
-		$errprefix = "file $filename line $lineno";
-
-		if ($line =~ m/^\s+(\S+)(\s+(.*\S))?\s*$/) {
-		    my ($k, $v) = ($1, $3);
-
-		    eval {
-			if ($unknown) {
-			    if (!defined($config->{$k})) {
-				$config->{$k} = $v;
-			    } else {
-				if (!ref($config->{$k})) {
-				    $config->{$k} = [$config->{$k}];
-				}
-				push $config->{$k}->@*, $v;
-			    }
-			} elsif ($is_array->($type, $k)) {
-			    $v = $plugin->check_value($type, $k, $v, $sectionId);
-			    $config->{$k} = [] if !defined($config->{$k});
-			    push $config->{$k}->@*, $v;
+	# Parse kv-pairs of section - will go on until empty line is encountered
+	while (my $section_line = $get_next_line->()) {
+	    if ($section_line =~ m/$re_kv_pair/) {
+		my ($key, $value) = ($1, $3);
+
+		eval {
+		    if ($is_unknown_type) {
+			if (!defined($config->{$key})) {
+			    $config->{$key} = $value;
 			} else {
-			    die "duplicate attribute\n" if defined($config->{$k});
-			    $v = $plugin->check_value($type, $k, $v, $sectionId);
-			    $config->{$k} = $v;
+			    $config->{$key} = [$config->{$key}] if !ref($config->{$key});
+			    push $config->{$key}->@*, $value;
 			}
-		    };
-		    if (my $err = $@) {
-			warn "$errprefix (section '$sectionId') - unable to parse value of '$k': $err";
-			push $errors->@*, {
-			    context => $errprefix,
-			    section => $sectionId,
-			    key => $k,
-			    err => $err,
-			};
+		    } elsif ($is_array->($type, $key)) {
+			$value = $plugin->check_value($type, $key, $value, $section_id);
+			$config->{$key} = [] if !defined($config->{$key});
+			push $config->{$key}->@*, $value;
+		    } else {
+			die "duplicate attribute\n" if defined($config->{$key});
+			$value = $plugin->check_value($type, $key, $value, $section_id);
+			$config->{$key} = $value;
 		    }
-
-		} else {
-		    warn "$errprefix (section '$sectionId') - ignore config line: $line\n";
+		};
+		if (my $err = $@) {
+		    warn "$errprefix (section '$section_id') - unable to parse value of '$key': $err";
+		    push $errors->@*, {
+			context => $errprefix,
+			section => $section_id,
+			key => $key,
+			err => $err,
+		    };
 		}
 	    }
+	}
 
-	    if ($unknown) {
-		$config->{type} = $type;
-		$ids->{$sectionId} = $config;
-		$order->{$sectionId} = $pri++;
-	    } elsif (!$skip && $type && $plugin && $config) {
-		$config->{type} = $type;
-		if (!$unknown) {
-		    $config = eval { $config = $plugin->check_config($sectionId, $config, 1, 1); };
-		    warn "$errprefix (skip section '$sectionId'): $@" if $@;
-		}
-		$ids->{$sectionId} = $config;
-		$order->{$sectionId} = $pri++;
+	if ($is_unknown_type || ($type && $plugin && $config)) {
+	    $config->{type} = $type;
+
+	    if (!$is_unknown_type) {
+		$config = eval { $config = $plugin->check_config($section_id, $config, 1, 1); };
+		warn "$errprefix (skip section '$section_id'): $@" if $@;
 	    }
 
-	} else {
-	    warn "$errprefix - ignore config line: $line\n";
+	    $ids->{$section_id} = $config;
+	    $order->{$section_id} = $current_section_no++;
 	}
     }
 
     my $cfg = {
 	ids => $ids,
 	order => $order,
-	digest => $digest
+	digest => $digest,
     };
+
     $cfg->{errors} = $errors if scalar($errors->@*) > 0;
 
     return $cfg;
-- 
2.39.2