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

[pve-devel] [PATCH common v2] 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 similar to
"cb2646c7b4974e33f4148752deec71f0d589b0f3" in proxmox-section-config.
This way we can write unknown section out again like we parsed it.

Add a regression test for an unknown field not in the schema.

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)
(Reported in the forum: https://forum.proxmox.com/threads/132091)

Suggested-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* only convert to array on the second occurance of a key
  (more future proof and does not need a change in the testcase)
* add test case that would have triggered the original issue
* improved commit message

 src/PVE/SectionConfig.pm    | 24 +++++++++++++++++-------
 test/section_config_test.pl |  2 ++
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index 2b75e6a..5690476 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -369,17 +369,22 @@ 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) {
+			    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;
 			} 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 +530,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..ccb364d 100755
--- a/test/section_config_test.pl
+++ b/test/section_config_test.pl
@@ -218,6 +218,7 @@ my $with_unknown_data = {
 	invalid => {
 	    type => 'bad',
 	    common => 'omg',
+	    unknownfield => 'shouldnotbehere',
 	},
     },
     order => enum(qw(t1 t2 invalid t3)),
@@ -234,6 +235,7 @@ one: t2
 
 bad: invalid
 	common omg
+	unknownfield shouldnotbehere
 
 two: t3
 	field2 5
-- 
2.30.2

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

[Fiona Ebner]

Am 16.08.23 um 11:09 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 similar to
> "cb2646c7b4974e33f4148752deec71f0d589b0f3" in proxmox-section-config.
> This way we can write unknown section out again like we parsed it.
> 
> Add a regression test for an unknown field not in the schema.
> 
> 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)
> (Reported in the forum: https://forum.proxmox.com/threads/132091)
> 
> Suggested-by: Fiona Ebner <f.ebner@proxmox.com>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---

applied, thanks! As a follow-up, I added an actual array in the unknown
section to test for that too.