public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH common v2] SectionConfig: fix handling unknown sections
@ 2023-08-16  9:09 Dominik Csapak
  2023-08-16  9:32 ` [pve-devel] applied: " Fiona Ebner
  0 siblings, 1 reply; 2+ messages in thread
From: Dominik Csapak @ 2023-08-16  9:09 UTC (permalink / raw)
  To: pve-devel

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





^ permalink raw reply	[flat|nested] 2+ messages in thread

* [pve-devel] applied: [PATCH common v2] SectionConfig: fix handling unknown sections
  2023-08-16  9:09 [pve-devel] [PATCH common v2] SectionConfig: fix handling unknown sections Dominik Csapak
@ 2023-08-16  9:32 ` Fiona Ebner
  0 siblings, 0 replies; 2+ messages in thread
From: Fiona Ebner @ 2023-08-16  9:32 UTC (permalink / raw)
  To: Proxmox VE development discussion, Dominik Csapak

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.




^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2023-08-16  9:32 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-16  9:09 [pve-devel] [PATCH common v2] SectionConfig: fix handling unknown sections Dominik Csapak
2023-08-16  9:32 ` [pve-devel] applied: " Fiona Ebner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal