[pve-devel] [PATCH v1 pve-common 3/3] section config: clean up parser logic
Fabian Grünbichler
f.gruenbichler at proxmox.com
Tue Jun 11 12:46:04 CEST 2024
On June 4, 2024 11:28 am, Max Carrara wrote:
> 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.
it also mixes deeper changes with things like variable renaming, which
makes it unnecessarily hard to review..
>
> Signed-off-by: Max Carrara <m.carrara at proxmox.com>
> ---
> src/PVE/SectionConfig.pm | 189 ++++++++++++++++++++-------------------
> 1 file changed, 98 insertions(+), 91 deletions(-)
>
> diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
> index a6b0183..30faaa4 100644
> --- a/src/PVE/SectionConfig.pm
> +++ b/src/PVE/SectionConfig.pm
> @@ -1014,25 +1014,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*$/x;
I am not sure this is really more readable? especially in a RE that is
basically only concerned with whitespace.. and for a RE that is only
used once..
>
> my $ids = {};
> my $order = {};
> -
> - $raw = '' if !defined($raw);
> -
> my $digest = Digest::SHA::sha1_hex($raw);
>
> - my $pri = 1;
> + my $current_order = 1;
this is actually a worse name than before. this would be something like
current_index? current_position? section_no?
> + 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;
what do we gain from this?
>
> my $is_array = sub {
> my ($type, $key) = @_;
> @@ -1043,106 +1044,112 @@ 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;
> + }
> +
> + # 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);
>
> - 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;
> + 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_order++;
> }
> }
>
> my $cfg = {
> ids => $ids,
> order => $order,
> - digest => $digest
> + digest => $digest,
> };
> - $cfg->{errors} = $errors if scalar($errors->@*) > 0;
> +
> + $cfg->{errors} = \@errors if scalar(@errors) > 0;
>
> return $cfg;
> }
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
More information about the pve-devel
mailing list