[pve-devel] [PATCH common] SectionConfig: fix handling unknown sections

[pve-devel] [PATCH common] SectionConfig: fix handling unknown sections

[Dominik Csapak]

if we're parsing an unknown section, we cannot check the schema with
`is_array` to check if it's an array type or not, thus we have to
handle that separately.

fix this by handling data in unknown sections like an array for all
analogous to "cb2646c7b4974e33f4148752deec71f0d589b0f3" in
proxmox-section-config. This way we can write unknown section out again
like we parsed it.

we have to adapt a single test case, which is ok since that is in an
`invalid` section of a config anyway.

This fixes an issue, where calling `qm destroy ID --purge` removed much
of the configs ob backup jobs (since there we parse an 'unknown' section
and run into the `is_array` error)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/SectionConfig.pm    | 20 ++++++++++++--------
 test/section_config_test.pl |  2 +-
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index 2b75e6a..0ccbda7 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -369,17 +369,16 @@ sub parse_config {
 		    my ($k, $v) = ($1, $3);
 
 		    eval {
-			if ($is_array->($type, $k)) {
-			    if (!$unknown) {
-				$v = $plugin->check_value($type, $k, $v, $sectionId);
-			    }
+			if ($unknown) {
+			    $config->{$k} = [] if !defined($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;
 			} else {
 			    die "duplicate attribute\n" if defined($config->{$k});
-			    if (!$unknown) {
-				$v = $plugin->check_value($type, $k, $v, $sectionId);
-			    }
+			    $v = $plugin->check_value($type, $k, $v, $sectionId);
 			    $config->{$k} = $v;
 			}
 		    };
@@ -525,7 +524,12 @@ sub write_config {
 		next if defined($done_hash->{$k});
 		$done_hash->{$k} = 1;
 		my $v = $scfg->{$k};
-		$data .= "\t$k $v\n";
+		my $ref = ref($v);
+		if (defined($ref) && $ref eq 'ARRAY') {
+		    $data .= "\t$k $_\n" for $v->@*;
+		} else {
+		    $data .= "\t$k $v\n";
+		}
 	    }
 	    $out .= "$data\n";
 	    next;
diff --git a/test/section_config_test.pl b/test/section_config_test.pl
index 02242bc..d574150 100755
--- a/test/section_config_test.pl
+++ b/test/section_config_test.pl
@@ -217,7 +217,7 @@ my $with_unknown_data = {
 	},
 	invalid => {
 	    type => 'bad',
-	    common => 'omg',
+	    common => ['omg'],
 	},
     },
     order => enum(qw(t1 t2 invalid t3)),
-- 
2.30.2

Re: [pve-devel] [PATCH common] SectionConfig: fix handling unknown sections

[Fiona Ebner]

Am 16.08.23 um 09:41 schrieb Dominik Csapak:
> if we're parsing an unknown section, we cannot check the schema with
> `is_array` to check if it's an array type or not, thus we have to
> handle that separately.
> 
> fix this by handling data in unknown sections like an array for all
> analogous to "cb2646c7b4974e33f4148752deec71f0d589b0f3" in
> proxmox-section-config. This way we can write unknown section out again
> like we parsed it.

Thank you for tackling this!

As briefly discussed off-list, there, we only start interpreting data in
unknown sections for keys appearing multiple times as an array. While it
shouldn't make a difference if all we do with data in unknown sections
is write it back out, the fact that most sections are not arrays makes
it feel a bit more future-proof to do the same here.

> 
> we have to adapt a single test case, which is ok since that is in an
> `invalid` section of a config anyway.
> 
> This fixes an issue, where calling `qm destroy ID --purge` removed much
> of the configs ob backup jobs (since there we parse an 'unknown' section
> and run into the `is_array` error)

Reference https://forum.proxmox.com/threads/132091

> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---

(...)

> diff --git a/test/section_config_test.pl b/test/section_config_test.pl
> index 02242bc..d574150 100755
> --- a/test/section_config_test.pl
> +++ b/test/section_config_test.pl
> @@ -217,7 +217,7 @@ my $with_unknown_data = {
>  	},
>  	invalid => {
>  	    type => 'bad',
> -	    common => 'omg',
> +	    common => ['omg'],
>  	},
>      },
>      order => enum(qw(t1 t2 invalid t3)),

So this test doesn't expose the issue just because the "common" property
is already defined. Please add a second test which uses an unknown
property to expose the issue so we'd notice any future regression.