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

[pve-devel] [PATCH common/http/guest-common/qemu-server v2] 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')

additionally, we (incidentally?) allowed the same format for the regular
'-list' formats, and the gui makes heavy use of this (iow. sending
arrays which get converted to \0 byte separated strings)

so we also auto-convert in some circumstances (which we want to
deprecate in a future version)

changes from v1:
* rebased on master
* incorporated feedback from wolfang
* added auto conversion of string <-> array in appropriate situations
* added tests for above
* omit applied manager patch

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-guest-common depends on new (applied) 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      |  61 ++++++++++++++++++----
 src/PVE/SectionConfig.pm    |  42 +++++++++++++--
 test/Makefile               |   9 +++-
 test/api_parameter_test.pl  | 100 ++++++++++++++++++++++++++++++++++++
 test/section_config_test.pl |  26 ++++++++++
 6 files changed, 237 insertions(+), 27 deletions(-)
 create mode 100755 test/api_parameter_test.pl

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 v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config

[Dominik Csapak]

a few 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
* the biggest point: in the RESTHandler, to be compatible with the
  current gui behavior, we have to rewrite two parameter types:
  - when the api defines a '-list' format for a string type, but we get
    a list (because of the changes in http-server), we join the list
    with a comma into a string
  - when the api defines an 'array' type, but we get a scalar value,
    wrap the value in an array (because for www-form-urlencoded, you
    cannot send an array with a single value) add tests for this
    behavior, some of which we want to deprecate and remove in the
    future

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* include wolfangs feedback
* include auto-conversion from string <-> list where appropriate and add
  tests for it

 src/PVE/JSONSchema.pm      |  12 +++++
 src/PVE/RESTHandler.pm     |  61 ++++++++++++++++++----
 test/Makefile              |   9 +++-
 test/api_parameter_test.pl | 100 +++++++++++++++++++++++++++++++++++++
 4 files changed, 172 insertions(+), 10 deletions(-)
 create mode 100755 test/api_parameter_test.pl

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 db86af2..369e302 100644
--- a/src/PVE/RESTHandler.pm
+++ b/src/PVE/RESTHandler.pm
@@ -426,6 +426,56 @@ sub find_handler {
     return ($handler_class, $method_info);
 }
 
+my $untaint_recursive;
+
+$untaint_recursive = sub {
+    my ($param) = @_;
+
+    my $ref = ref($param);
+    if ($ref eq 'HASH') {
+	$param->{$_} = $untaint_recursive->($param->{$_}) for keys $param->%*;
+    } elsif ($ref eq 'ARRAY') {
+	for (my $i = 0; $i < scalar($param->@*); $i++) {
+	    $param->[$i] = $untaint_recursive->($param->[$i]);
+	}
+    } else {
+	if (defined($param)) {
+	    my ($newval) = $param =~ /^(.*)$/s;
+	    $param = $newval;
+	}
+    }
+
+    return $param;
+};
+
+# convert arrays to strings where we expect a '-list' format and convert scalar
+# values to arrays when we expect an array (because of www-form-urlencoded)
+#
+# only on the top level, since www-form-urlencoded cannot be nested anyway
+#
+# FIXME: change gui/api calls to not rely on this during 8.x, mark the
+# behaviour deprecated with 9.x, and remove it with 10.x
+my $convert_params = sub { my ($param, $schema) = @_;
+
+    return if !$schema->{properties};
+    return if (ref($param) // '') ne 'HASH';
+
+    for my $key (keys $schema->{properties}->%*) {
+	if (my $value = $param->{$key}) {
+	    my $type = $schema->{properties}->{$key}->{type} // '';
+	    my $format = $schema->{properties}->{$key}->{format} // '';
+	    my $ref = ref($value);
+	    if ($ref eq 'ARRAY' && $type eq 'string' && $format =~ m/-list$/) {
+		$param->{$key} = join(',', $value->@*);
+	    } elsif (!$ref && $type eq 'array') {
+		$param->{$key} = [$value];
+	    }
+	}
+    }
+
+    return $param;
+};
+
 sub handle {
     my ($self, $info, $param, $result_verification) = @_;
 
@@ -437,17 +487,10 @@ sub handle {
 
     if (my $schema = $info->{parameters}) {
 	# warn "validate ". Dumper($param}) . "\n" . Dumper($schema);
+	$param = $convert_params->($param, $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->{'extra-args'} = [map { /^(.*)$/ } @$extra] if $extra;
+	$param = $untaint_recursive->($param);
     }
 
     my $result = $func->($param); # the actual API code execution call
diff --git a/test/Makefile b/test/Makefile
index 5c64867..82f40ab 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -1,4 +1,11 @@
 SUBDIRS = etc_network_interfaces
+TESTS = lock_file.test			\
+	calendar_event_test.test	\
+	convert_size_test.test		\
+	procfs_tests.test		\
+	format_test.test		\
+	section_config_test.test	\
+	api_parameter_test.test		\
 
 all:
 
@@ -6,7 +13,7 @@ all:
 
 export PERLLIB=../src
 
-check: lock_file.test calendar_event_test.test convert_size_test.test procfs_tests.test format_test.test section_config_test.test
+check: $(TESTS)
 	for d in $(SUBDIRS); do $(MAKE) -C $$d check; done
 
 %.test: %.pl
diff --git a/test/api_parameter_test.pl b/test/api_parameter_test.pl
new file mode 100755
index 0000000..7ade386
--- /dev/null
+++ b/test/api_parameter_test.pl
@@ -0,0 +1,100 @@
+#!/usr/bin/perl
+package PVE::TestAPIParameters;
+
+# Tests the automatic conversion of -list and array parameter types
+
+use strict;
+use warnings;
+
+use lib '../src';
+
+use PVE::RESTHandler;
+use PVE::JSONSchema;
+
+use Test::More;
+
+use base qw(PVE::RESTHandler);
+
+my $setup = [
+    {
+	name => 'list-format-with-list',
+	parameter => {
+	    type => 'string',
+	    format => 'pve-configid-list',
+	},
+	value => "foo,bar",
+	'value-expected' => "foo,bar",
+    },
+    {
+	name => 'array-format-with-array',
+	parameter => {
+	    type => 'array',
+	    items => {
+		type => 'string',
+		format => 'pve-configid',
+	    },
+	},
+	value => ['foo', 'bar'],
+	'value-expected' => ['foo', 'bar'],
+    },
+    # TODO: below behaviour should be deprecated with 9.x and fail with 10.x
+    {
+	name => 'list-format-with-alist',
+	parameter => {
+	    type => 'string',
+	    format => 'pve-configid-list',
+	},
+	value => "foo\0bar",
+	'value-expected' => "foo\0bar",
+    },
+    {
+	name => 'array-format-with-non-array',
+	parameter => {
+	    type => 'array',
+	    items => {
+		type => 'string',
+		format => 'pve-configid',
+	    },
+	},
+	value => "foo",
+	'value-expected' => ['foo'],
+    },
+    {
+	name => 'list-format-with-array',
+	parameter => {
+	    type => 'string',
+	    format => 'pve-configid-list',
+	},
+	value => ['foo', 'bar'],
+	'value-expected' => "foo,bar",
+    },
+];
+
+for my $data ($setup->@*) {
+    __PACKAGE__->register_method({
+	name => $data->{name},
+	path => $data->{name},
+	method => 'POST',
+	parameters => {
+	    additionalProperties => 0,
+	    properties => {
+		param => $data->{parameter},
+	    },
+	},
+	returns => { type => 'null' },
+	code => sub {
+	    my ($param) = @_;
+	    return $param->{param};
+	}
+    });
+
+    my ($handler, $info) = __PACKAGE__->find_handler('POST', $data->{name});
+    my $param = {
+	param => $data->{value},
+    };
+
+    my $res = $handler->handle($info, $param);
+    is_deeply($res, $data->{'value-expected'}, $data->{name});
+}
+
+done_testing();
-- 
2.30.2

[pve-devel] [PATCH common v2 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:
* remove unnecessary code move
 src/PVE/SectionConfig.pm    | 42 ++++++++++++++++++++++++++++++++-----
 test/section_config_test.pl | 26 +++++++++++++++++++++++
 2 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index f36cede..97492db 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -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 v2 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>
---
 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 http-server v2 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 v2 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>
---
 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 v2 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 v2 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 common v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config

[Thomas Lamprecht]

Am 06/06/2023 um 10:39 schrieb Dominik Csapak:
> a few 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
> * the biggest point: in the RESTHandler, to be compatible with the
>   current gui behavior, we have to rewrite two parameter types:
>   - when the api defines a '-list' format for a string type, but we get
>     a list (because of the changes in http-server), we join the list
>     with a comma into a string
>   - when the api defines an 'array' type, but we get a scalar value,
>     wrap the value in an array (because for www-form-urlencoded, you
>     cannot send an array with a single value) add tests for this
>     behavior, some of which we want to deprecate and remove in the
>     future
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v1:
> * include wolfangs feedback
> * include auto-conversion from string <-> list where appropriate and add
>   tests for it
> 
>  src/PVE/JSONSchema.pm      |  12 +++++
>  src/PVE/RESTHandler.pm     |  61 ++++++++++++++++++----
>  test/Makefile              |   9 +++-
>  test/api_parameter_test.pl | 100 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 172 insertions(+), 10 deletions(-)
>  create mode 100755 test/api_parameter_test.pl
> 
> 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') {

code style, and can be fixed up:
for multi-line if's place the closing parenthesis and opening block { on it's own line:

It also doesn't hurt to move all expressions part of the condition in a separate line
(albeit that part is not a rule in our style guide):

if (
    $schema->{properties}->{$key}
    && $schema->{properties}->{$key}->{type} eq 'array'
) {
    # ...

> +
> +		if (defined($cfg->{$key})) {
> +		    push $cfg->{$key}->@*, $value;
> +		} else {
> +		    $cfg->{$key} = [$value];
> +		}

Could be written shorter, but just fine as above

$cfg->{$key} //= [];
push $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 db86af2..369e302 100644
> --- a/src/PVE/RESTHandler.pm
> +++ b/src/PVE/RESTHandler.pm
> @@ -426,6 +426,56 @@ sub find_handler {
>      return ($handler_class, $method_info);
>  }
>  
> +my $untaint_recursive;

I got flash backs w.r.t. refcount cycles here keeping all variables, and thus memory
inside the body alive forever, don't we need a weaken?

E.g., like we had to do in PVE::Status::Graphite's assemble.
> +
> +$untaint_recursive = sub {
> +    my ($param) = @_;
> +
> +    my $ref = ref($param);
> +    if ($ref eq 'HASH') {
> +	$param->{$_} = $untaint_recursive->($param->{$_}) for keys $param->%*;
> +    } elsif ($ref eq 'ARRAY') {
> +	for (my $i = 0; $i < scalar($param->@*); $i++) {
> +	    $param->[$i] = $untaint_recursive->($param->[$i]);
> +	}
> +    } else {
> +	if (defined($param)) {

could be merged into upper branch as elsif, but no hard feelings.

> +	    my ($newval) = $param =~ /^(.*)$/s;
> +	    $param = $newval;
> +	}
> +    }
> +
> +    return $param;
> +};
> +
> +# convert arrays to strings where we expect a '-list' format and convert scalar
> +# values to arrays when we expect an array (because of www-form-urlencoded)
> +#
> +# only on the top level, since www-form-urlencoded cannot be nested anyway
> +#
> +# FIXME: change gui/api calls to not rely on this during 8.x, mark the
> +# behaviour deprecated with 9.x, and remove it with 10.x
> +my $convert_params = sub { my ($param, $schema) = @_;

please keep the method paramethers on it's own line.

Also, maybe go for a more telling names, as convert_params could mean everytrhing
and nothing ^^ 



> +
> +    return if !$schema->{properties};
> +    return if (ref($param) // '') ne 'HASH';

doesn't this breaks the assignment when used below? I.e.,:

$param = $convert_params->($param, $schema);

or messes with silenting parameters sent to a endpoint without properties, which would
create an extra param error otherwise?

> +
> +    for my $key (keys $schema->{properties}->%*) {
> +	if (my $value = $param->{$key}) {
> +	    my $type = $schema->{properties}->{$key}->{type} // '';
> +	    my $format = $schema->{properties}->{$key}->{format} // '';
> +	    my $ref = ref($value);
> +	    if ($ref eq 'ARRAY' && $type eq 'string' && $format =~ m/-list$/) {

Should this also check ref to not be undef, i.e.

if ($ref && $ref eq 'ARRAY' && ...


> +		$param->{$key} = join(',', $value->@*);
> +	    } elsif (!$ref && $type eq 'array') {
> +		$param->{$key} = [$value];
> +	    }
> +	}
> +    }
> +
> +    return $param;
> +};
> +
>  sub handle {
>      my ($self, $info, $param, $result_verification) = @_;
>  
> @@ -437,17 +487,10 @@ sub handle {
>  
>      if (my $schema = $info->{parameters}) {
>  	# warn "validate ". Dumper($param}) . "\n" . Dumper($schema);
> +	$param = $convert_params->($param, $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->{'extra-args'} = [map { /^(.*)$/ } @$extra] if $extra;
> +	$param = $untaint_recursive->($param);
>      }
>  
>      my $result = $func->($param); # the actual API code execution call

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

[Dominik Csapak]

On 6/6/23 11:12, Thomas Lamprecht wrote:
> Am 06/06/2023 um 10:39 schrieb Dominik Csapak:
>> a few 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
>> * the biggest point: in the RESTHandler, to be compatible with the
>>    current gui behavior, we have to rewrite two parameter types:
>>    - when the api defines a '-list' format for a string type, but we get
>>      a list (because of the changes in http-server), we join the list
>>      with a comma into a string
>>    - when the api defines an 'array' type, but we get a scalar value,
>>      wrap the value in an array (because for www-form-urlencoded, you
>>      cannot send an array with a single value) add tests for this
>>      behavior, some of which we want to deprecate and remove in the
>>      future
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> changes from v1:
>> * include wolfangs feedback
>> * include auto-conversion from string <-> list where appropriate and add
>>    tests for it
>>
>>   src/PVE/JSONSchema.pm      |  12 +++++
>>   src/PVE/RESTHandler.pm     |  61 ++++++++++++++++++----
>>   test/Makefile              |   9 +++-
>>   test/api_parameter_test.pl | 100 +++++++++++++++++++++++++++++++++++++
>>   4 files changed, 172 insertions(+), 10 deletions(-)
>>   create mode 100755 test/api_parameter_test.pl
>>
>> 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') {
> 
> code style, and can be fixed up:
> for multi-line if's place the closing parenthesis and opening block { on it's own line:
> 
> It also doesn't hurt to move all expressions part of the condition in a separate line
> (albeit that part is not a rule in our style guide):
> 
> if (
>      $schema->{properties}->{$key}
>      && $schema->{properties}->{$key}->{type} eq 'array'
> ) {
>      # ...
> 

sure, sorry

>> +
>> +		if (defined($cfg->{$key})) {
>> +		    push $cfg->{$key}->@*, $value;
>> +		} else {
>> +		    $cfg->{$key} = [$value];
>> +		}
> 
> Could be written shorter, but just fine as above
> 
> $cfg->{$key} //= [];
> push $cfg->{$key}->@*, $value;

yours is shorter and still understandable
> 
>> +		next;
>> +	    }
>>   	    $cfg->{$key} = $value;
>>   	} else {
>>   	    warn "ignore config line: $line\n"
>> diff --git a/src/PVE/RESTHandler.pm b/src/PVE/RESTHandler.pm
>> index db86af2..369e302 100644
>> --- a/src/PVE/RESTHandler.pm
>> +++ b/src/PVE/RESTHandler.pm
>> @@ -426,6 +426,56 @@ sub find_handler {
>>       return ($handler_class, $method_info);
>>   }
>>   
>> +my $untaint_recursive;
> 
> I got flash backs w.r.t. refcount cycles here keeping all variables, and thus memory
> inside the body alive forever, don't we need a weaken?
> 
> E.g., like we had to do in PVE::Status::Graphite's assemble.

mhmm isn't that because there we use variables from outside the
function? here we only use the parameters themselves

anyway the solution there is to set the sub to undef after use, but
we can do that here only if we move the sub into the regular
function

i can also make it a proper sub if that's better?

how can i test for these things properly?

>> +
>> +$untaint_recursive = sub {
>> +    my ($param) = @_;
>> +
>> +    my $ref = ref($param);
>> +    if ($ref eq 'HASH') {
>> +	$param->{$_} = $untaint_recursive->($param->{$_}) for keys $param->%*;
>> +    } elsif ($ref eq 'ARRAY') {
>> +	for (my $i = 0; $i < scalar($param->@*); $i++) {
>> +	    $param->[$i] = $untaint_recursive->($param->[$i]);
>> +	}
>> +    } else {
>> +	if (defined($param)) {
> 
> could be merged into upper branch as elsif, but no hard feelings.
> 
>> +	    my ($newval) = $param =~ /^(.*)$/s;
>> +	    $param = $newval;
>> +	}
>> +    }
>> +
>> +    return $param;
>> +};
>> +
>> +# convert arrays to strings where we expect a '-list' format and convert scalar
>> +# values to arrays when we expect an array (because of www-form-urlencoded)
>> +#
>> +# only on the top level, since www-form-urlencoded cannot be nested anyway
>> +#
>> +# FIXME: change gui/api calls to not rely on this during 8.x, mark the
>> +# behaviour deprecated with 9.x, and remove it with 10.x
>> +my $convert_params = sub { my ($param, $schema) = @_;
> 
> please keep the method paramethers on it's own line.

oops, one shift+j to many without noticing^^

> 
> Also, maybe go for a more telling names, as convert_params could mean everytrhing
> and nothing ^^
> 

sure, any suggestions? ;)

> 
> 
>> +
>> +    return if !$schema->{properties};
>> +    return if (ref($param) // '') ne 'HASH';
> 
> doesn't this breaks the assignment when used below? I.e.,:
> 
> $param = $convert_params->($param, $schema);
> 
> or messes with silenting parameters sent to a endpoint without properties, which would
> create an extra param error otherwise?

yes, we have to return the original param here in both cases

> 
>> +
>> +    for my $key (keys $schema->{properties}->%*) {
>> +	if (my $value = $param->{$key}) {
>> +	    my $type = $schema->{properties}->{$key}->{type} // '';
>> +	    my $format = $schema->{properties}->{$key}->{format} // '';
>> +	    my $ref = ref($value);
>> +	    if ($ref eq 'ARRAY' && $type eq 'string' && $format =~ m/-list$/) {
> 
> Should this also check ref to not be undef, i.e.
> 
> if ($ref && $ref eq 'ARRAY' && ...
> 

yes

> 
>> +		$param->{$key} = join(',', $value->@*);
>> +	    } elsif (!$ref && $type eq 'array') {
>> +		$param->{$key} = [$value];
>> +	    }
>> +	}
>> +    }
>> +
>> +    return $param;
>> +};
>> +
>>   sub handle {
>>       my ($self, $info, $param, $result_verification) = @_;
>>   
>> @@ -437,17 +487,10 @@ sub handle {
>>   
>>       if (my $schema = $info->{parameters}) {
>>   	# warn "validate ". Dumper($param}) . "\n" . Dumper($schema);
>> +	$param = $convert_params->($param, $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->{'extra-args'} = [map { /^(.*)$/ } @$extra] if $extra;
>> +	$param = $untaint_recursive->($param);
>>       }
>>   
>>       my $result = $func->($param); # the actual API code execution call
> 

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

[Thomas Lamprecht]

Am 06/06/2023 um 11:41 schrieb Dominik Csapak:
>>>   +my $untaint_recursive;
>>
>> I got flash backs w.r.t. refcount cycles here keeping all variables, and thus memory
>> inside the body alive forever, don't we need a weaken?
>>
>> E.g., like we had to do in PVE::Status::Graphite's assemble.
> 
> mhmm isn't that because there we use variables from outside the
> function? here we only use the parameters themselves

I'm not 100% sure about the details, but since then, seeing something like
this pattern triggers my cycle instincts, I'd like to have that checked out
closely.

> 
> anyway the solution there is to set the sub to undef after use, but
> we can do that here only if we move the sub into the regular
> function
> 
> i can also make it a proper sub if that's better?
> 



> how can i test for these things properly?

easiest: pass large hashes and loop, then see if memory goes up. For metrics
back then you could see the RSS of pbvestatd grow by ~ the metric data size
every update.


What I also used back then, IIRC, was the Devel::Cycle module, it should give
you a more specific answer if there's a cycle, but naturally has no idea what
the practical implications are.


>>> +# convert arrays to strings where we expect a '-list' format and convert scalar
>>> +# values to arrays when we expect an array (because of www-form-urlencoded)
>>> +#
>>> +# only on the top level, since www-form-urlencoded cannot be nested anyway
>>> +#
>>> +# FIXME: change gui/api calls to not rely on this during 8.x, mark the
>>> +# behaviour deprecated with 9.x, and remove it with 10.x
>>> +my $convert_params = sub { my ($param, $schema) = @_;
>>
>> please keep the method paramethers on it's own line.
> 
> oops, one shift+j to many without noticing^^
> 
>>
>> Also, maybe go for a more telling names, as convert_params could mean everytrhing
>> and nothing ^^
>>
> 
> sure, any suggestions? 😉

sure, lets start with what this actually does in more explicit steps:

1) normalize_form_data
still a bit general but we now know that it handles form data and normalizes it to a
single representation


2) normalize_param_list_to_array
A bit more telling about what happens.


3) convert_legacy_list_format_to_array
very telling, but as this is a internal helper, and thus not used elsewhere, that
wouldn't hurt, having "legacy" in it underlines that we want to drop it sometimes.

Personally, I'd favor 3) or 2).

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

[Dominik Csapak]

On 6/6/23 12:45, Thomas Lamprecht wrote:
> Am 06/06/2023 um 11:41 schrieb Dominik Csapak:
>>>>    +my $untaint_recursive;
>>>
>>> I got flash backs w.r.t. refcount cycles here keeping all variables, and thus memory
>>> inside the body alive forever, don't we need a weaken?
>>>
>>> E.g., like we had to do in PVE::Status::Graphite's assemble.
>>
>> mhmm isn't that because there we use variables from outside the
>> function? here we only use the parameters themselves
> 
> I'm not 100% sure about the details, but since then, seeing something like
> this pattern triggers my cycle instincts, I'd like to have that checked out
> closely.
> 

sure

>>
>> anyway the solution there is to set the sub to undef after use, but
>> we can do that here only if we move the sub into the regular
>> function
>>
>> i can also make it a proper sub if that's better?
>>
> 
> 
> 
>> how can i test for these things properly?
> 
> easiest: pass large hashes and loop, then see if memory goes up. For metrics
> back then you could see the RSS of pbvestatd grow by ~ the metric data size
> every update.
> 
> 
> What I also used back then, IIRC, was the Devel::Cycle module, it should give
> you a more specific answer if there's a cycle, but naturally has no idea what
> the practical implications are.

i wrote a small program where i copied the function to and did basically:

---8<---
use Storable qw(dclone);

my $normalize;
$normalize = sub {...};

my $data = /* create large hash here, with nested data */;

while(1) {
     my $newdata = dclone($data);
     $newdata = $normalize->($newdata);
}
--->8---


executed it and monitored the rss usage, while letting it run for multiple minutes
the memory usage increased after the initial creation of the hash, and the first
dclone, but not after.

is that a sufficient test?

> 
>>>> +# convert arrays to strings where we expect a '-list' format and convert scalar
>>>> +# values to arrays when we expect an array (because of www-form-urlencoded)
>>>> +#
>>>> +# only on the top level, since www-form-urlencoded cannot be nested anyway
>>>> +#
>>>> +# FIXME: change gui/api calls to not rely on this during 8.x, mark the
>>>> +# behaviour deprecated with 9.x, and remove it with 10.x
>>>> +my $convert_params = sub { my ($param, $schema) = @_;
>>>
>>> please keep the method paramethers on it's own line.
>>
>> oops, one shift+j to many without noticing^^
>>
>>>
>>> Also, maybe go for a more telling names, as convert_params could mean everytrhing
>>> and nothing ^^
>>>
>>
>> sure, any suggestions? 😉
> 
> sure, lets start with what this actually does in more explicit steps:
> 
> 1) normalize_form_data
> still a bit general but we now know that it handles form data and normalizes it to a
> single representation
> 
> 
> 2) normalize_param_list_to_array
> A bit more telling about what happens.
> 
> 
> 3) convert_legacy_list_format_to_array
> very telling, but as this is a internal helper, and thus not used elsewhere, that
> wouldn't hurt, having "legacy" in it underlines that we want to drop it sometimes.
> 
> Personally, I'd favor 3) or 2).

mhmm both are ok for me, but it also does the reverse
(convert an array to a legacy list so to speak)

maybe i'd use something like 'normalize_legacy_param_formats' ?
it's a bit more general than 2,3 but still has legacy in its name...

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

[Thomas Lamprecht]

Am 06/06/2023 um 13:19 schrieb Dominik Csapak:
> ---8<---
> use Storable qw(dclone);
> 
> my $normalize;
> $normalize = sub {...};
> 
> my $data = /* create large hash here, with nested data */;
> 
> while(1) {
>     my $newdata = dclone($data);
>     $newdata = $normalize->($newdata);
> }
> --->8---
> 
> 
> executed it and monitored the rss usage, while letting it run for multiple minutes
> the memory usage increased after the initial creation of the hash, and the first
> dclone, but not after.
> 
> is that a sufficient test?

yeah, seems fine enough to me.


> maybe i'd use something like 'normalize_legacy_param_formats' ?

would be an improvement over the original convert_params and fine enough for me.

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

[Wolfgang Bumiller]

On Tue, Jun 06, 2023 at 12:45:57PM +0200, Thomas Lamprecht wrote:
> Am 06/06/2023 um 11:41 schrieb Dominik Csapak:
> >>>   +my $untaint_recursive;
> >>
> >> I got flash backs w.r.t. refcount cycles here keeping all variables, and thus memory
> >> inside the body alive forever, don't we need a weaken?
> >>
> >> E.g., like we had to do in PVE::Status::Graphite's assemble.
> > 
> > mhmm isn't that because there we use variables from outside the
> > function? here we only use the parameters themselves
> 
> I'm not 100% sure about the details, but since then, seeing something like
> this pattern triggers my cycle instincts, I'd like to have that checked out
> closely.

I *do* prefer `my sub` these days.
However, for recursive subs you need to `use feature 'current_sub'` to
avoid ... well... leaks ;-)

So:

    my sub untaint_recursive : prototype($) {
        use feature 'current_sub';

        my ($arg) = @_;

        ...
        # For recursion:
        __SUB__->($stuff);
        ...
    }

Given that this function shouldn't be leaky, you could keep it, or even
pre-declare the sub to allow recursion:

    my sub untaint_recursive : prototype($);
    sub untaint_recursive : prototype($) {
        <regular code>
    }

however, `perlsub` explicitly states that this, too, can leak ;-)