From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 77E1C1FF38E for ; Tue, 11 Jun 2024 12:45:36 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E8DEC35242; Tue, 11 Jun 2024 12:46:10 +0200 (CEST) Date: Tue, 11 Jun 2024 12:46:04 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20240604092850.126083-1-m.carrara@proxmox.com> <20240604092850.126083-4-m.carrara@proxmox.com> In-Reply-To: <20240604092850.126083-4-m.carrara@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1718102096.jr51lrt2bo.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.057 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com, sectionconfig.pm] Subject: Re: [pve-devel] [PATCH v1 pve-common 3/3] section config: clean up parser logic X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 > --- > 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@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