[pve-devel] [PATCH common/manager/http/guest/qemu-server] schema/config array support

[pve-devel] [PATCH common/manager/http/guest/qemu-server] schema/config array support

[Dominik Csapak]

and removal of the '-alist' format

This series aims to implement array support for the api and
(section)config and remove the support for the (rarely used) '-alist'
formats.

currently sending arrays over the api (by sending a parameter multiple
times with form-urlencoded) results in the api call getting a \0
separated string, which it must split with e.g. split_list.

Instead, we want to simply specify an 'array' type for the api.

We only ever used '-alist' in two places:
* 'exclude-path' config/parameter for vzdump
* 'command' parameter for qemu guest agent exec via api (cli has it's
  own implementation with 'extra-args')

The dependcies/breaks are like this:

pve-http-server 1/2 is rather independent (did encounter this during
testing, and the current behaviour is imho wrong)

pve-manager patch is built to use the new and old formats of
'exclude-path', so this should be safe to apply first

pve-guest-common depends on pve-manager, pve-http-server and pve-common 1/3

qemu-sever depends on pve-http-server and pve-common 1/3

pve-http-server does not technically break the older pve-guest-common
and qemu-server versions, but the parameters would need to be
encoded/sent differently than before (iow. the api user would have to
send the commands/paths \0 separated already instead of repeating the
parameters (form-urlencoded arrays))

pve-common 2/3 depend on all above

pve-common 3/3 depend on all above and it breaks every version that uses
an '-alist' format (so older pve-guest-common + qemu-server)

we can of course omit pve-common 3/3 and leave support in the
jsonschema, but imho it's really weird to use and has no real upsides
versus using simply an array type

pve-common:

Dominik Csapak (3):
  JSONSchema: add support for array parameter in api calls, cli and
    config
  section config: implement array support
  JSONSchema: disable '-alist' format

 src/PVE/JSONSchema.pm       | 26 ++++++++-------
 src/PVE/RESTHandler.pm      | 41 ++++++++++++++++++++----
 src/PVE/SectionConfig.pm    | 64 +++++++++++++++++++++++++++----------
 test/section_config_test.pl | 26 +++++++++++++++
 4 files changed, 122 insertions(+), 35 deletions(-)

pve-manager:

Dominik Csapak (1):
  vzdump: prepare 'exclude-path' for array format

 PVE/VZDump.pm | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

pve-http-server:

Dominik Csapak (2):
  proxy request: forward json content type and parameters
  use proper arrays for array parameter

 src/PVE/APIServer/AnyEvent.pm | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

pve-guest-common

Dominik Csapak (1):
  vzdump: change 'exclude-path' from alist to an array format

 src/PVE/VZDump/Common.pm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

qemu-server:

Dominik Csapak (1):
  api: switch agent api call to 'array' type

 PVE/API2/Qemu/Agent.pm | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH common 1/3] JSONSchema: add support for array parameter in api calls, cli and config

[Dominik Csapak]

only three small things were missing for it to work:
* on the cli, we have to get the option as an array if the type is an array
* the untainting must be done recursively, otherwise, the regex matching
  converts an array hash into the string 'ARRAY(0x123412341234)'
* JSONSchema::parse_config did not handle array formats specially, but
  we want to allow to specify them multiple time

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/JSONSchema.pm  | 12 ++++++++++++
 src/PVE/RESTHandler.pm | 41 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index 527e409..526fc2b 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -1709,6 +1709,8 @@ sub get_options {
 	} else {
 	    if ($pd->{format} && $pd->{format} =~ m/-a?list/) {
 		push @getopt, "$prop=s@";
+	    } elsif ($pd->{type} eq 'array') {
+		push @getopt, "$prop=s@";
 	    } else {
 		push @getopt, "$prop=s";
 	    }
@@ -1869,6 +1871,16 @@ sub parse_config : prototype($$$;$) {
 
 		$value = parse_boolean($value) // $value;
 	    }
+	    if ($schema->{properties}->{$key} &&
+		$schema->{properties}->{$key}->{type} eq 'array') {
+
+		if (defined($cfg->{$key})) {
+		    push $cfg->{$key}->@*, $value;
+		} else {
+		    $cfg->{$key} = [$value];
+		}
+		next;
+	    }
 	    $cfg->{$key} = $value;
 	} else {
 	    warn "ignore config line: $line\n"
diff --git a/src/PVE/RESTHandler.pm b/src/PVE/RESTHandler.pm
index 944a04b..20714a5 100644
--- a/src/PVE/RESTHandler.pm
+++ b/src/PVE/RESTHandler.pm
@@ -426,6 +426,38 @@ sub find_handler {
     return ($handler_class, $method_info);
 }
 
+my $untaint_recursive;
+
+$untaint_recursive = sub {
+    my ($param) = @_;
+
+    my $ref = ref $param;
+    if ($ref eq 'HASH') {
+	while (my ($key, $val) = each %$param) {
+	    my $newval = $untaint_recursive->($val);
+	    if (defined($newval)) {
+		($param->{$key}) = $newval;
+	    } else {
+		$param->{$key} = undef;
+	    }
+	}
+    } elsif ($ref eq 'ARRAY') {
+	my $newparam = [];
+	for my $val (@$param) {
+	    my $newval = $untaint_recursive->($val);
+	    push @$newparam, $newval if defined($newval);
+	}
+	$param = $newparam;
+    } else {
+	if (defined($param)) {
+	    my ($newval) = $param =~ /^(.*)$/s;
+	    $param = $newval;
+	}
+    }
+
+    return $param;
+};
+
 sub handle {
     my ($self, $info, $param, $result_verification) = @_;
 
@@ -437,16 +469,11 @@ sub handle {
 
     if (my $schema = $info->{parameters}) {
 	# warn "validate ". Dumper($param}) . "\n" . Dumper($schema);
+
 	PVE::JSONSchema::validate($param, $schema);
 	# untaint data (already validated)
 	my $extra = delete $param->{'extra-args'};
-	while (my ($key, $val) = each %$param) {
-	    if (defined($val)) {
-		($param->{$key}) = $val =~ /^(.*)$/s;
-	    } else {
-		$param->{$key} = undef;
-	    }
-	}
+	$param = $untaint_recursive->($param);
 	$param->{'extra-args'} = [map { /^(.*)$/ } @$extra] if $extra;
     }
 
-- 
2.30.2

[pve-devel] [PATCH v2 common 2/3] section config: implement array support

[Dominik Csapak]

enables section configs in the style of:

----
type: id
    property value
    property value2
    property value3
----

can be combined with property strings

the provided create and update schema just pass through the array type
to the api, so the api call must always contain the complete array

also adds a test case for such array fields

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* don't special encode arrays anymore, but instead have always the whole
  array as parameter in the create/updateSchema
 src/PVE/SectionConfig.pm    | 64 +++++++++++++++++++++++++++----------
 test/section_config_test.pl | 26 +++++++++++++++
 2 files changed, 74 insertions(+), 16 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index f36cede..03f6a52 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -51,6 +51,17 @@ sub plugindata {
     return {};
 }
 
+my $copy_property = sub {
+    my ($src) = @_;
+
+    my $res = {};
+    foreach my $k (keys %$src) {
+	$res->{$k} = $src->{$k};
+    }
+
+    return $res;
+};
+
 sub createSchema {
     my ($class, $skip_type) = @_;
 
@@ -60,17 +71,6 @@ sub createSchema {
 
     my $props = {};
 
-    my $copy_property = sub {
-	my ($src) = @_;
-
-	my $res = {};
-	foreach my $k (keys %$src) {
-	    $res->{$k} = $src->{$k};
-	}
-
-	return $res;
-    };
-
     foreach my $p (keys %$propertyList) {
 	next if $skip_type && $p eq 'type';
 
@@ -254,7 +254,15 @@ sub check_value {
 
     if (!$skipSchemaCheck) {
 	my $errors = {};
-	PVE::JSONSchema::check_prop($value, $schema, '', $errors);
+
+	my $checkschema = $schema;
+
+	if ($ct eq 'array') {
+	    die "no item schema for array" if !defined($schema->{items});
+	    $checkschema = $schema->{items};
+	}
+
+	PVE::JSONSchema::check_prop($value, $checkschema, '', $errors);
 	if (scalar(keys %$errors)) {
 	    die "$errors->{$key}\n" if $errors->{$key};
 	    die "$errors->{_root}\n" if $errors->{_root};
@@ -311,6 +319,15 @@ sub parse_config {
 	}
     };
 
+    my $is_array = sub {
+	my ($type, $key) = @_;
+
+	my $schema = $pdata->{propertyList}->{$key};
+	die "unknown property type\n" if !$schema;
+
+	return $schema->{type} eq 'array';
+    };
+
     my $errors = [];
     while (@lines) {
 	my $line = $nextline->();
@@ -352,11 +369,19 @@ sub parse_config {
 		    my ($k, $v) = ($1, $3);
 
 		    eval {
-			die "duplicate attribute\n" if defined($config->{$k});
-			if (!$unknown) {
-			    $v = $plugin->check_value($type, $k, $v, $sectionId);
+			if ($is_array->($type, $k)) {
+			    if (!$unknown) {
+				$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);
+			    }
+			    $config->{$k} = $v;
 			}
-			$config->{$k} = $v;
 		    };
 		    if (my $err = $@) {
 			warn "$errprefix (section '$sectionId') - unable to parse value of '$k': $err";
@@ -448,6 +473,13 @@ my $format_config_line = sub {
     if ($ct eq 'boolean') {
 	return "\t$key " . ($value ? 1 : 0) . "\n"
 	    if defined($value);
+    } elsif ($ct eq 'array') {
+	die "property '$key' is not an array" if ref($value) ne 'ARRAY';
+	my $result = '';
+	for my $line ($value->@*) {
+	    $result .= "\t$key $line\n" if $value ne '';
+	}
+	return $result;
     } else {
 	return "\t$key $value\n" if "$value" ne '';
     }
diff --git a/test/section_config_test.pl b/test/section_config_test.pl
index 22a9643..02242bc 100755
--- a/test/section_config_test.pl
+++ b/test/section_config_test.pl
@@ -105,6 +105,25 @@ sub properties {
 	    minimum => 3,
 	    maximum => 9,
 	},
+	arrayfield => {
+	    description => "Array Field with property string",
+	    type => 'array',
+	    items => {
+		type => 'string',
+		description => 'a property string',
+		format => {
+		    subfield1 => {
+			type => 'string',
+			description => 'first subfield'
+		    },
+		    subfield2 => {
+			type => 'integer',
+			minimum => 0,
+			optional => 1,
+		    },
+		},
+	    },
+	},
     };
 }
 
@@ -113,6 +132,7 @@ sub options {
 	common => { optional => 1 },
 	field2 => {},
 	another => {},
+	arrayfield => { optional => 1 },
     };
 }
 
@@ -190,6 +210,10 @@ my $with_unknown_data = {
 	    type => 'two',
 	    field2 => 5,
 	    another => 'even more text',
+	    arrayfield => [
+		'subfield1=test,subfield2=2',
+		'subfield1=test2',
+	    ],
 	},
 	invalid => {
 	    type => 'bad',
@@ -214,6 +238,8 @@ bad: invalid
 two: t3
 	field2 5
 	another even more text
+	arrayfield subfield1=test,subfield2=2
+	arrayfield subfield1=test2
 EOF
 
 Conf->expect_fail('unknown-forbidden', $with_unknown_data, $with_unknown_text);
-- 
2.30.2

[pve-devel] [PATCH common 3/3] JSONSchema: disable '-alist' format

[Dominik Csapak]

this should not be needed anymore since we can now use a simple array
in the api instead

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
this breaks all packages which have an '-alist' format defined
 src/PVE/JSONSchema.pm | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index 526fc2b..b148ce9 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -795,7 +795,7 @@ sub check_format {
     return if $format eq 'regex';
 
     my $parsed;
-    $format =~ m/^(.*?)(?:-a?(list|opt))?$/;
+    $format =~ m/^(.*?)(?:-(list|opt))?$/;
     my ($format_name, $format_type) = ($1, $2 // 'none');
     my $registered = get_format($format_name);
     die "undefined format '$format'\n" if !$registered;
@@ -1707,7 +1707,7 @@ sub get_options {
 	} elsif ($pd->{type} eq 'boolean') {
 	    push @getopt, "$prop:s";
 	} else {
-	    if ($pd->{format} && $pd->{format} =~ m/-a?list/) {
+	    if ($pd->{format} && $pd->{format} =~ m/-list/) {
 		push @getopt, "$prop=s@";
 	    } elsif ($pd->{type} eq 'array') {
 		push @getopt, "$prop=s@";
@@ -1812,16 +1812,6 @@ sub get_options {
 		    # allow --vmid 100 --vmid 101 and --vmid 100,101
 		    # allow --dow mon --dow fri and --dow mon,fri
 		    $opts->{$p} = join(",", @{$opts->{$p}}) if ref($opts->{$p}) eq 'ARRAY';
-		} elsif ($pd->{format} =~ m/-alist/) {
-		    # we encode array as \0 separated strings
-		    # Note: CGI.pm also use this encoding
-		    if (scalar(@{$opts->{$p}}) != 1) {
-			$opts->{$p} = join("\0", @{$opts->{$p}});
-		    } else {
-			# st that split_list knows it is \0 terminated
-			my $v = $opts->{$p}->[0];
-			$opts->{$p} = "$v\0";
-		    }
 		}
 	    }
 	}
-- 
2.30.2

[pve-devel] [PATCH manager 1/1] vzdump: prepare 'exclude-path' for array format

[Dominik Csapak]

we want to move the 'exclude-path' to an array format (from 'string-alist')
prepare the code that it can be either a string or a list

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/VZDump.pm | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index a04837e7e..dde347562 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -279,7 +279,15 @@ sub read_vzdump_defaults {
     my $conf_schema = { type => 'object', properties => $confdesc_for_defaults };
     my $res = PVE::JSONSchema::parse_config($conf_schema, $fn, $raw);
     if (my $excludes = $res->{'exclude-path'}) {
-	$res->{'exclude-path'} = PVE::Tools::split_args($excludes);
+	if (ref($excludes) eq 'ARRAY') {
+	    my $list = [];
+	    for my $path ($excludes->@*) {
+		push $list->@*, PVE::Tools::split_args($path)->@*;
+	    }
+	    $res->{'exclude-path'} = $list;
+	} else {
+	    $res->{'exclude-path'} = PVE::Tools::split_args($excludes);
+	}
     }
     if (defined($res->{mailto})) {
 	my @mailto = split_list($res->{mailto});
@@ -1339,10 +1347,15 @@ sub option_exists {
 sub parse_mailto_exclude_path {
     my ($param) = @_;
 
-    # exclude-path list need to be 0 separated
+    # exclude-path list need to be 0 separated or be an array
     if (defined($param->{'exclude-path'})) {
-	my @expaths = split(/\0/, $param->{'exclude-path'} || '');
-	$param->{'exclude-path'} = [ @expaths ];
+	my $expaths;
+	if (ref($param->{'exclude-path'}) eq 'ARRAY') {
+	    $expaths = $param->{'exclude-path'};
+	} else {
+	    $expaths = [split(/\0/, $param->{'exclude-path'} || '')];
+	}
+	$param->{'exclude-path'} = $expaths;
     }
 
     if (defined($param->{mailto})) {
-- 
2.30.2

[pve-devel] [PATCH http-server 1/2] proxy request: forward json content type and parameters

[Dominik Csapak]

instead of always trying to encode them as x-www-form-urlencoded

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/APIServer/AnyEvent.pm | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index b2ae99b..e9d87b3 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -745,11 +745,16 @@ sub proxy_request {
 	my $content;
 
 	if  ($method eq 'POST' || $method eq 'PUT') {
-	    $headers->{'Content-Type'} = 'application/x-www-form-urlencoded';
-	    # use URI object to format application/x-www-form-urlencoded content.
-	    my $url = URI->new('http:');
-	    $url->query_form(%$params);
-	    $content = $url->query;
+	    if ($reqstate->{request}->header('Content-Type') =~ 'application/json') {
+		$headers->{'Content-Type'} = 'application/json';
+		$content = encode_json($params);
+	    } else {
+		$headers->{'Content-Type'} = 'application/x-www-form-urlencoded';
+		# use URI object to format application/x-www-form-urlencoded content.
+		my $url = URI->new('http:');
+		$url->query_form(%$params);
+		$content = $url->query;
+	    }
 	    if (defined($content)) {
 		$headers->{'Content-Length'} = length($content);
 	    }
-- 
2.30.2

[pve-devel] [PATCH http-server 2/2] use proper arrays for array parameter

[Dominik Csapak]

since there is no other way to get an array parameter when using
x-www-form-urlencoded content type

the previous format with \0 separated strings (known as '-alist' format)
should not be used anymore (in favor of the now supported arrays)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
this technically breaks the api use for '-alist' formats since the user
must \0 separate the entries themselves instead of giving them multiple
times
 src/PVE/APIServer/AnyEvent.pm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index e9d87b3..e56a196 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -850,7 +850,12 @@ sub decode_urlencoded {
 	    $v = Encode::decode('utf8', $v);
 
 	    if (defined(my $old = $res->{$k})) {
-		$v = "$old\0$v";
+		if (ref($old) eq 'ARRAY') {
+		    push @$old, $v;
+		    $v = $old;
+		} else {
+		    $v = [$old, $v];
+		}
 	    }
 	}
 
-- 
2.30.2

[pve-devel] [PATCH guest-common 1/1] vzdump: change 'exclude-path' from alist to an array format

[Dominik Csapak]

to get rid of the '-alist' format

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/VZDump/Common.pm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/PVE/VZDump/Common.pm b/src/PVE/VZDump/Common.pm
index 4b0e8e0..7d3c311 100644
--- a/src/PVE/VZDump/Common.pm
+++ b/src/PVE/VZDump/Common.pm
@@ -154,11 +154,14 @@ my $confdesc = {
 	optional => 1,
     },
     'exclude-path' => {
-	type => 'string', format => 'string-alist',
+	type => 'array',
 	description => "Exclude certain files/directories (shell globs)." .
 	    " Paths starting with '/' are anchored to the container's root, " .
 	    " other paths match relative to each subdirectory.",
 	optional => 1,
+	items => {
+	    type => 'string',
+	},
     },
     mailto => {
 	type => 'string',
@@ -422,7 +425,7 @@ sub command_line {
 	my $v = $param->{$p};
 	my $pd = $confdesc->{$p} || die "no such vzdump option '$p'\n";
 	if ($p eq 'exclude-path') {
-	    foreach my $path (split(/\0/, $v || '')) {
+	    foreach my $path (@$v) {
 		$cmd .= " --$p " . PVE::Tools::shellquote($path);
 	    }
 	} else {
-- 
2.30.2

[pve-devel] [PATCH qemu-server 1/1] api: switch agent api call to 'array' type

[Dominik Csapak]

we don't want to use the '-alist' formats anymore in favor of real arrays

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/API2/Qemu/Agent.pm | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/PVE/API2/Qemu/Agent.pm b/PVE/API2/Qemu/Agent.pm
index 5ff1fa9d..dceee770 100644
--- a/PVE/API2/Qemu/Agent.pm
+++ b/PVE/API2/Qemu/Agent.pm
@@ -274,10 +274,12 @@ __PACKAGE__->register_method({
 	    vmid => get_standard_option('pve-vmid', {
 		    completion => \&PVE::QemuServer::complete_vmid_running }),
 	    command => {
-		type => 'string',
-		format => 'string-alist',
-		description => 'The command as a list of program + arguments',
-		optional => 1,
+		type => 'array',
+		description => 'The command as a list of program + arguments.',
+		items => {
+		    format => 'string',
+		    description => 'A single part of the program + arguments.',
+		}
 	    },
 	    'input-data' => {
 		type => 'string',
@@ -300,10 +302,7 @@ __PACKAGE__->register_method({
 	my ($param) = @_;
 
 	my $vmid = $param->{vmid};
-	my $cmd = undef;
-	if ($param->{command}) {
-	    $cmd = [PVE::Tools::split_list($param->{command})];
-	}
+	my $cmd = $param->{command};
 
 	my $res = PVE::QemuServer::Agent::qemu_exec($vmid, $param->{'input-data'}, $cmd);
 	return $res;
-- 
2.30.2

Re: [pve-devel] [PATCH v2 common 2/3] section config: implement array support

[Wolfgang Bumiller]

On Fri, May 12, 2023 at 02:23:49PM +0200, Dominik Csapak wrote:
> enables section configs in the style of:
> 
> ----
> type: id
>     property value
>     property value2
>     property value3
> ----
> 
> can be combined with property strings
> 
> the provided create and update schema just pass through the array type
> to the api, so the api call must always contain the complete array
> 
> also adds a test case for such array fields
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v1:
> * don't special encode arrays anymore, but instead have always the whole
>   array as parameter in the create/updateSchema
>  src/PVE/SectionConfig.pm    | 64 +++++++++++++++++++++++++++----------
>  test/section_config_test.pl | 26 +++++++++++++++
>  2 files changed, 74 insertions(+), 16 deletions(-)
> 
> diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
> index f36cede..03f6a52 100644
> --- a/src/PVE/SectionConfig.pm
> +++ b/src/PVE/SectionConfig.pm
> @@ -51,6 +51,17 @@ sub plugindata {
>      return {};
>  }
>  
> +my $copy_property = sub {

If you're moving it, maybe *re*move it and change the single
caller of it to just do `{ %$src }`, since that's all this does ;-)

> +    my ($src) = @_;
> +
> +    my $res = {};
> +    foreach my $k (keys %$src) {
> +	$res->{$k} = $src->{$k};
> +    }
> +
> +    return $res;
> +};
> +
>  sub createSchema {
>      my ($class, $skip_type) = @_;
>  

Re: [pve-devel] [PATCH v2 common 2/3] section config: implement array support

[Dominik Csapak]

On 5/15/23 11:07, Wolfgang Bumiller wrote:
> On Fri, May 12, 2023 at 02:23:49PM +0200, Dominik Csapak wrote:
>> enables section configs in the style of:
>>
>> ----
>> type: id
>>      property value
>>      property value2
>>      property value3
>> ----
>>
>> can be combined with property strings
>>
>> the provided create and update schema just pass through the array type
>> to the api, so the api call must always contain the complete array
>>
>> also adds a test case for such array fields
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> changes from v1:
>> * don't special encode arrays anymore, but instead have always the whole
>>    array as parameter in the create/updateSchema
>>   src/PVE/SectionConfig.pm    | 64 +++++++++++++++++++++++++++----------
>>   test/section_config_test.pl | 26 +++++++++++++++
>>   2 files changed, 74 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
>> index f36cede..03f6a52 100644
>> --- a/src/PVE/SectionConfig.pm
>> +++ b/src/PVE/SectionConfig.pm
>> @@ -51,6 +51,17 @@ sub plugindata {
>>       return {};
>>   }
>>   
>> +my $copy_property = sub {
> 
> If you're moving it, maybe *re*move it and change the single
> caller of it to just do `{ %$src }`, since that's all this does ;-)
> 

oops, i used it in the v1, now not anymore, forgot to remove the hunks ;)
but can do in a v3 as a separate patch

Re: [pve-devel] [PATCH manager 1/1] vzdump: prepare 'exclude-path' for array format

[Wolfgang Bumiller]

On Fri, May 12, 2023 at 02:23:51PM +0200, Dominik Csapak wrote:
> we want to move the 'exclude-path' to an array format (from 'string-alist')
> prepare the code that it can be either a string or a list
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/VZDump.pm | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index a04837e7e..dde347562 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -279,7 +279,15 @@ sub read_vzdump_defaults {
>      my $conf_schema = { type => 'object', properties => $confdesc_for_defaults };
>      my $res = PVE::JSONSchema::parse_config($conf_schema, $fn, $raw);
>      if (my $excludes = $res->{'exclude-path'}) {
> -	$res->{'exclude-path'} = PVE::Tools::split_args($excludes);
> +	if (ref($excludes) eq 'ARRAY') {
> +	    my $list = [];
> +	    for my $path ($excludes->@*) {
> +		push $list->@*, PVE::Tools::split_args($path)->@*;

With this being an array, I don't think it makes sense to call
`split_args()` on the individual items?

> +	    }
> +	    $res->{'exclude-path'} = $list;
> +	} else {
> +	    $res->{'exclude-path'} = PVE::Tools::split_args($excludes);
> +	}
>      }
>      if (defined($res->{mailto})) {
>  	my @mailto = split_list($res->{mailto});
> @@ -1339,10 +1347,15 @@ sub option_exists {
>  sub parse_mailto_exclude_path {
>      my ($param) = @_;
>  
> -    # exclude-path list need to be 0 separated
> +    # exclude-path list need to be 0 separated or be an array
>      if (defined($param->{'exclude-path'})) {
> -	my @expaths = split(/\0/, $param->{'exclude-path'} || '');
> -	$param->{'exclude-path'} = [ @expaths ];
> +	my $expaths;
> +	if (ref($param->{'exclude-path'}) eq 'ARRAY') {
> +	    $expaths = $param->{'exclude-path'};
> +	} else {
> +	    $expaths = [split(/\0/, $param->{'exclude-path'} || '')];
> +	}
> +	$param->{'exclude-path'} = $expaths;
>      }
>  
>      if (defined($param->{mailto})) {
> -- 
> 2.30.2

Re: [pve-devel] [PATCH manager 1/1] vzdump: prepare 'exclude-path' for array format

[Dominik Csapak]

On 6/5/23 09:36, Wolfgang Bumiller wrote:
> On Fri, May 12, 2023 at 02:23:51PM +0200, Dominik Csapak wrote:
>> we want to move the 'exclude-path' to an array format (from 'string-alist')
>> prepare the code that it can be either a string or a list
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   PVE/VZDump.pm | 21 +++++++++++++++++----
>>   1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
>> index a04837e7e..dde347562 100644
>> --- a/PVE/VZDump.pm
>> +++ b/PVE/VZDump.pm
>> @@ -279,7 +279,15 @@ sub read_vzdump_defaults {
>>       my $conf_schema = { type => 'object', properties => $confdesc_for_defaults };
>>       my $res = PVE::JSONSchema::parse_config($conf_schema, $fn, $raw);
>>       if (my $excludes = $res->{'exclude-path'}) {
>> -	$res->{'exclude-path'} = PVE::Tools::split_args($excludes);
>> +	if (ref($excludes) eq 'ARRAY') {
>> +	    my $list = [];
>> +	    for my $path ($excludes->@*) {
>> +		push $list->@*, PVE::Tools::split_args($path)->@*;
> 
> With this being an array, I don't think it makes sense to call
> `split_args()` on the individual items?

actually we have to do this to keep compatibility with current configs:

currently users can put things like this in the config:

exclude-path: /foo /bar

and both paths will be excluded

if we omit the splitting we'd have to rewrite the configs to:

exclude-path: /foo
exclude-path: /bar

while we could do that, i opted here for the simpler approach to handle
the lines the same, without need to break existing configs
and/or rewriting configs on upgrade

> 
>> +	    }
>> +	    $res->{'exclude-path'} = $list;
>> +	} else {
>> +	    $res->{'exclude-path'} = PVE::Tools::split_args($excludes);
>> +	}
>>       }
>>       if (defined($res->{mailto})) {
>>   	my @mailto = split_list($res->{mailto});
>> @@ -1339,10 +1347,15 @@ sub option_exists {
>>   sub parse_mailto_exclude_path {
>>       my ($param) = @_;
>>   
>> -    # exclude-path list need to be 0 separated
>> +    # exclude-path list need to be 0 separated or be an array
>>       if (defined($param->{'exclude-path'})) {
>> -	my @expaths = split(/\0/, $param->{'exclude-path'} || '');
>> -	$param->{'exclude-path'} = [ @expaths ];
>> +	my $expaths;
>> +	if (ref($param->{'exclude-path'}) eq 'ARRAY') {
>> +	    $expaths = $param->{'exclude-path'};
>> +	} else {
>> +	    $expaths = [split(/\0/, $param->{'exclude-path'} || '')];
>> +	}
>> +	$param->{'exclude-path'} = $expaths;
>>       }
>>   
>>       if (defined($param->{mailto})) {
>> -- 
>> 2.30.2

Re: [pve-devel] [PATCH manager 1/1] vzdump: prepare 'exclude-path' for array format

[Wolfgang Bumiller]

On Mon, Jun 05, 2023 at 09:54:49AM +0200, Dominik Csapak wrote:
> On 6/5/23 09:36, Wolfgang Bumiller wrote:
> > On Fri, May 12, 2023 at 02:23:51PM +0200, Dominik Csapak wrote:
> > > we want to move the 'exclude-path' to an array format (from 'string-alist')
> > > prepare the code that it can be either a string or a list
> > > 
> > > Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> > > ---
> > >   PVE/VZDump.pm | 21 +++++++++++++++++----
> > >   1 file changed, 17 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> > > index a04837e7e..dde347562 100644
> > > --- a/PVE/VZDump.pm
> > > +++ b/PVE/VZDump.pm
> > > @@ -279,7 +279,15 @@ sub read_vzdump_defaults {
> > >       my $conf_schema = { type => 'object', properties => $confdesc_for_defaults };
> > >       my $res = PVE::JSONSchema::parse_config($conf_schema, $fn, $raw);
> > >       if (my $excludes = $res->{'exclude-path'}) {
> > > -	$res->{'exclude-path'} = PVE::Tools::split_args($excludes);
> > > +	if (ref($excludes) eq 'ARRAY') {
> > > +	    my $list = [];
> > > +	    for my $path ($excludes->@*) {
> > > +		push $list->@*, PVE::Tools::split_args($path)->@*;
> > 
> > With this being an array, I don't think it makes sense to call
> > `split_args()` on the individual items?
> 
> actually we have to do this to keep compatibility with current configs:
> 
> currently users can put things like this in the config:
> 
> exclude-path: /foo /bar
> 
> and both paths will be excluded
> 
> if we omit the splitting we'd have to rewrite the configs to:
> 
> exclude-path: /foo
> exclude-path: /bar
> 
> while we could do that, i opted here for the simpler approach to handle
> the lines the same, without need to break existing configs
> and/or rewriting configs on upgrade

Right, that is unfortunate.
Maybe we can at some point figure out a syntax hint to change this
behavior, as I find this a bit unintuitive when actually using multiple
entries.

[pve-devel] applied: [PATCH manager 1/1] vzdump: prepare 'exclude-path' for array format

[Wolfgang Bumiller]

applied and added a followup commenting the `split_args` on the array
case

Re: [pve-devel] [PATCH common 1/3] JSONSchema: add support for array parameter in api calls, cli and config

[Wolfgang Bumiller]

On Fri, May 12, 2023 at 02:23:48PM +0200, Dominik Csapak wrote:
> only three small things were missing for it to work:
> * on the cli, we have to get the option as an array if the type is an array
> * the untainting must be done recursively, otherwise, the regex matching
>   converts an array hash into the string 'ARRAY(0x123412341234)'
> * JSONSchema::parse_config did not handle array formats specially, but
>   we want to allow to specify them multiple time
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/JSONSchema.pm  | 12 ++++++++++++
>  src/PVE/RESTHandler.pm | 41 ++++++++++++++++++++++++++++++++++-------
>  2 files changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
> index 527e409..526fc2b 100644
> --- a/src/PVE/JSONSchema.pm
> +++ b/src/PVE/JSONSchema.pm
> @@ -1709,6 +1709,8 @@ sub get_options {
>  	} else {
>  	    if ($pd->{format} && $pd->{format} =~ m/-a?list/) {
>  		push @getopt, "$prop=s@";
> +	    } elsif ($pd->{type} eq 'array') {
> +		push @getopt, "$prop=s@";
>  	    } else {
>  		push @getopt, "$prop=s";
>  	    }
> @@ -1869,6 +1871,16 @@ sub parse_config : prototype($$$;$) {
>  
>  		$value = parse_boolean($value) // $value;
>  	    }
> +	    if ($schema->{properties}->{$key} &&
> +		$schema->{properties}->{$key}->{type} eq 'array') {
> +
> +		if (defined($cfg->{$key})) {
> +		    push $cfg->{$key}->@*, $value;
> +		} else {
> +		    $cfg->{$key} = [$value];
> +		}
> +		next;
> +	    }
>  	    $cfg->{$key} = $value;
>  	} else {
>  	    warn "ignore config line: $line\n"
> diff --git a/src/PVE/RESTHandler.pm b/src/PVE/RESTHandler.pm
> index 944a04b..20714a5 100644
> --- a/src/PVE/RESTHandler.pm
> +++ b/src/PVE/RESTHandler.pm
> @@ -426,6 +426,38 @@ sub find_handler {
>      return ($handler_class, $method_info);
>  }
>  
> +my $untaint_recursive;
> +
> +$untaint_recursive = sub {
> +    my ($param) = @_;
> +
> +    my $ref = ref $param;
> +    if ($ref eq 'HASH') {
> +	while (my ($key, $val) = each %$param) {
> +	    my $newval = $untaint_recursive->($val);
> +	    if (defined($newval)) {

if defined, assign untainted value

> +		($param->{$key}) = $newval;
> +	    } else {
> +		$param->{$key} = undef;

if ==undef, assign undef

> +	    }
> +	}

So this entire case could just be:

    $param->{$_} = $untaint_recursive->($param->{$_) for keys %$param;

can it not?

However - here, we're explicitly assigning `undef`s, but in the array
case below...

> +    } elsif ($ref eq 'ARRAY') {
> +	my $newparam = [];
> +	for my $val (@$param) {
> +	    my $newval = $untaint_recursive->($val);
> +	    push @$newparam, $newval if defined($newval);

...here we're skipping undef.
I'd like comments explaining the cases in within this sub.

Further, the 'hash' case replaces the existing hash, while the array
case produces a _new_ array, which could make an actual difference for
the toplevel caller of this method.

The caller below expects a return value for a hash, discarding the
original, so this might be fine, but I'd prefer to either stay
consistent, or a have a big fat side effect warning comment on top of
the sub.

> +	}
> +	$param = $newparam;
> +    } else {
> +	if (defined($param)) {
> +	    my ($newval) = $param =~ /^(.*)$/s;
> +	    $param = $newval;
> +	}
> +    }
> +
> +    return $param;
> +};
> +
>  sub handle {
>      my ($self, $info, $param, $result_verification) = @_;
>  
> @@ -437,16 +469,11 @@ sub handle {
>  
>      if (my $schema = $info->{parameters}) {
>  	# warn "validate ". Dumper($param}) . "\n" . Dumper($schema);
> +
>  	PVE::JSONSchema::validate($param, $schema);
>  	# untaint data (already validated)
>  	my $extra = delete $param->{'extra-args'};
> -	while (my ($key, $val) = each %$param) {
> -	    if (defined($val)) {
> -		($param->{$key}) = $val =~ /^(.*)$/s;
> -	    } else {
> -		$param->{$key} = undef;
> -	    }
> -	}
> +	$param = $untaint_recursive->($param);

And after this, I think the line below can be dropped?

>  	$param->{'extra-args'} = [map { /^(.*)$/ } @$extra] if $extra;
>      }
>  
> -- 
> 2.30.2

Re: [pve-devel] [PATCH common 3/3] JSONSchema: disable '-alist' format

[Wolfgang Bumiller]

On Fri, May 12, 2023 at 02:23:50PM +0200, Dominik Csapak wrote:
> this should not be needed anymore since we can now use a simple array
> in the api instead
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>

Acked-by: Wolfgang Bumiller <w.bumiller@proxmox.com>

Re: [pve-devel] [PATCH guest-common 1/1] vzdump: change 'exclude-path' from alist to an array formaty

[Wolfgang Bumiller]

On Fri, May 12, 2023 at 02:23:54PM +0200, Dominik Csapak wrote:
> to get rid of the '-alist' format
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>

Acked-by: Wolfgang Bumiller <w.bumiller@proxmox.com>

Re: [pve-devel] [PATCH qemu-server 1/1] api: switch agent api call to 'array' type

[Wolfgang Bumiller]

On Fri, May 12, 2023 at 02:23:55PM +0200, Dominik Csapak wrote:
> we don't want to use the '-alist' formats anymore in favor of real arrays
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>

Acked-by: Wolfgang Bumiller <w.bumiller@proxmox.com>

Re: [pve-devel] [PATCH http-server 1/2] proxy request: forward json content type and parameters

[Wolfgang Bumiller]

On Fri, May 12, 2023 at 02:23:52PM +0200, Dominik Csapak wrote:
> instead of always trying to encode them as x-www-form-urlencoded
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>

Acked-by: Wolfgang Bumiller <w.bumiller@proxmox.com>

for both patches