Re: [pve-devel] [PATCH v1 pve-common 3/3] section config: clean up parser logic

["Max Carrara" <m.carrara@proxmox.com>]

On Tue Jun 11, 2024 at 12:46 PM CEST, Fabian Grünbichler wrote:
> 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..

Should've mentioned that; mea culpa. Will see if I can retain the old
names, otherwise will mention / list the renamed variables.

>
> > 
> > Signed-off-by: Max Carrara <m.carrara@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..

I mean I could drop the `x` flag if you'd like, just thought that it
would make it a little easier to parse... Though, the reason why I'm
personally a fan of assigning regexes to variables is because it makes
the code more expressive IMO. If someone who usually doesn't deal with
regexes has to read this code, the variable name describes what the
regex does. That's mainly the reason why I put it there.

>
> >  
> >      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?

Hm, agreed - those are actually better. Will incorporate in v2, thanks!

>
> > +    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 intention here was to work on the array directly and then return it
as a reference below (if there are any errors) rather than working with
a reference from the get-go, having to deref whenever there's a `push`.

I guess it's more a personal preference of mine, I just find it better
to work with. Can keep the array ref in v2 though, doesn't matter that
much. :P

>
> >  
> >      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@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> > 
> > 
> > 
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel