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

[pve-devel] [PATCH common/http/guest-common/qemu-server v3] 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 v2:
* renamed 'convert_param' sub
* made it properly recursive with 'use feature current_sub'
  (thanks wolfgang)
* style fixes according to thomas
* added wolfangs acked-by i missed last 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       |  25 ++++-----
 src/PVE/RESTHandler.pm      |  62 ++++++++++++++++++----
 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 v3 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 v2:
* style fixes (thanks thomas)
* proper recursive sub with 'use feature 'current_sub'
* renamed 'conver_param' to something more explicit
 src/PVE/JSONSchema.pm      |  11 ++++
 src/PVE/RESTHandler.pm     |  62 +++++++++++++++++++----
 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..1867279 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,15 @@ sub parse_config : prototype($$$;$) {
 
 		$value = parse_boolean($value) // $value;
 	    }
+	    if (
+		$schema->{properties}->{$key}
+		&& $schema->{properties}->{$key}->{type} eq 'array'
+	    ) {
+
+		$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..6fd70c8 100644
--- a/src/PVE/RESTHandler.pm
+++ b/src/PVE/RESTHandler.pm
@@ -426,6 +426,57 @@ sub find_handler {
     return ($handler_class, $method_info);
 }
 
+my sub untaint_recursive : prototype($) {
+    use feature 'current_sub';
+
+    my ($param) = @_;
+
+    my $ref = ref($param);
+    if ($ref eq 'HASH') {
+	$param->{$_} = __SUB__->($param->{$_}) for keys $param->%*;
+    } elsif ($ref eq 'ARRAY') {
+	for (my $i = 0; $i < scalar($param->@*); $i++) {
+	    $param->[$i] = __SUB__->($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 $normalize_legacy_param_formats = sub {
+    my ($param, $schema) = @_;
+
+    return $param if !$schema->{properties};
+    return $param 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 && $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 +488,10 @@ sub handle {
 
     if (my $schema = $info->{parameters}) {
 	# warn "validate ". Dumper($param}) . "\n" . Dumper($schema);
+	$param = $normalize_legacy_param_formats->($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] applied: [PATCH common v3 1/3] JSONSchema: add support for array parameter in api calls, cli and config

[Wolfgang Bumiller]

applied this one & bumped

[pve-devel] [PATCH common v3 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>
---
 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 v3 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

Acked-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
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 1867279..4ecb4d9 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 v3 1/2] proxy request: forward json content type and parameters

[Dominik Csapak]

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

Acked-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
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] applied: [PATCH http-server v3 1/2] proxy request: forward json content type and parameters

[Wolfgang Bumiller]

applied both & bumped dependency for pve-common

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

[Thomas Lamprecht]

Am 07/06/2023 um 13:23 schrieb Wolfgang Bumiller:
> applied both & bumped dependency for pve-common
> 

Isn't this breaking older common too?

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

[Dominik Csapak]

On 6/7/23 13:47, Thomas Lamprecht wrote:
> Am 07/06/2023 um 13:23 schrieb Wolfgang Bumiller:
>> applied both & bumped dependency for pve-common
>>
> 
> Isn't this breaking older common too?
> 

no it just forwards the parameters to either the daemon or the other 
nodes as json if the proxy received it as json, should not have
any other effects...

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

[Thomas Lamprecht]

Am 07/06/2023 um 13:49 schrieb Dominik Csapak:
> 
> 
> On 6/7/23 13:47, Thomas Lamprecht wrote:
>> Am 07/06/2023 um 13:23 schrieb Wolfgang Bumiller:
>>> applied both & bumped dependency for pve-common
>>>
>>
>> Isn't this breaking older common too?
>>
> 
> no it just forwards the parameters to either the daemon or the other nodes as json if the proxy received it as json, should not have
> any other effects...

not this one 2/2, just replied here as it was the applied patch

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

[Wolfgang Bumiller]

On Wed, Jun 07, 2023 at 01:59:26PM +0200, Thomas Lamprecht wrote:
> Am 07/06/2023 um 13:49 schrieb Dominik Csapak:
> > 
> > 
> > On 6/7/23 13:47, Thomas Lamprecht wrote:
> >> Am 07/06/2023 um 13:23 schrieb Wolfgang Bumiller:
> >>> applied both & bumped dependency for pve-common
> >>>
> >>
> >> Isn't this breaking older common too?
> >>
> > 
> > no it just forwards the parameters to either the daemon or the other nodes as json if the proxy received it as json, should not have
> > any other effects...
> 
> not this one 2/2, just replied here as it was the applied patch

Meh, I suppose technically it does, but requires a... broken(?) client
to trigger it, since no API endpoints with arrays which reach this case
existed before?

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

[Thomas Lamprecht]

Am 07/06/2023 um 14:08 schrieb Wolfgang Bumiller:
> On Wed, Jun 07, 2023 at 01:59:26PM +0200, Thomas Lamprecht wrote:
>> Am 07/06/2023 um 13:49 schrieb Dominik Csapak:
>>>
>>>
>>> On 6/7/23 13:47, Thomas Lamprecht wrote:
>>>> Am 07/06/2023 um 13:23 schrieb Wolfgang Bumiller:
>>>>> applied both & bumped dependency for pve-common
>>>>>
>>>>
>>>> Isn't this breaking older common too?
>>>>
>>>
>>> no it just forwards the parameters to either the daemon or the other nodes as json if the proxy received it as json, should not have
>>> any other effects...
>>
>> not this one 2/2, just replied here as it was the applied patch
> 
> Meh, I suppose technically it does, but requires a... broken(?) client
> to trigger it, since no API endpoints with arrays which reach this case
> existed before?
> 

But Dominik writes in a reply to 2/2:

> 
> this patch breaks pve-common without the common 1/3 applied
> since the gui sends arrays when the api expects a '-list'
> and the  api treats '-list' and '-alist' the same 

I.e., this breaks manager, and the workaround for that is in common, so either
http-server breaks older manager (i.e., the version before increasing the
versioned dependency for libpve-common-perl >= the one containing 1/3 in manager.

Alternatively this could break older libpve-common-perl, enforcing that manager
always gets a workable set of packages for it's state.

But not doing any breaks, if Dominik's reply is correct, is just plain wrong.

(Potentially) breaking external client's is then something else, not fully ideal
but if risk was evaluated and deemed impractical then OK, otherwise it's a rather
less ideal..

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

[Wolfgang Bumiller]

On Wed, Jun 07, 2023 at 02:14:37PM +0200, Thomas Lamprecht wrote:
> Am 07/06/2023 um 14:08 schrieb Wolfgang Bumiller:
> > On Wed, Jun 07, 2023 at 01:59:26PM +0200, Thomas Lamprecht wrote:
> >> Am 07/06/2023 um 13:49 schrieb Dominik Csapak:
> >>>
> >>>
> >>> On 6/7/23 13:47, Thomas Lamprecht wrote:
> >>>> Am 07/06/2023 um 13:23 schrieb Wolfgang Bumiller:
> >>>>> applied both & bumped dependency for pve-common
> >>>>>
> >>>>
> >>>> Isn't this breaking older common too?
> >>>>
> >>>
> >>> no it just forwards the parameters to either the daemon or the other nodes as json if the proxy received it as json, should not have
> >>> any other effects...
> >>
> >> not this one 2/2, just replied here as it was the applied patch
> > 
> > Meh, I suppose technically it does, but requires a... broken(?) client
> > to trigger it, since no API endpoints with arrays which reach this case
> > existed before?
> > 
> 
> But Dominik writes in a reply to 2/2:
> 
> > 
> > this patch breaks pve-common without the common 1/3 applied
> > since the gui sends arrays when the api expects a '-list'
> > and the  api treats '-list' and '-alist' the same 
> 
> I.e., this breaks manager, and the workaround for that is in common, so either
> http-server breaks older manager (i.e., the version before increasing the
> versioned dependency for libpve-common-perl >= the one containing 1/3 in manager.
> 
> Alternatively this could break older libpve-common-perl, enforcing that manager
> always gets a workable set of packages for it's state.

Right, I'll add a breaks entry for common.

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

[Wolfgang Bumiller]

On Wed, Jun 07, 2023 at 02:23:36PM +0200, Wolfgang Bumiller wrote:
> On Wed, Jun 07, 2023 at 02:14:37PM +0200, Thomas Lamprecht wrote:
> > Am 07/06/2023 um 14:08 schrieb Wolfgang Bumiller:
> > > On Wed, Jun 07, 2023 at 01:59:26PM +0200, Thomas Lamprecht wrote:
> > >> Am 07/06/2023 um 13:49 schrieb Dominik Csapak:
> > >>>
> > >>>
> > >>> On 6/7/23 13:47, Thomas Lamprecht wrote:
> > >>>> Am 07/06/2023 um 13:23 schrieb Wolfgang Bumiller:
> > >>>>> applied both & bumped dependency for pve-common
> > >>>>>
> > >>>>
> > >>>> Isn't this breaking older common too?
> > >>>>
> > >>>
> > >>> no it just forwards the parameters to either the daemon or the other nodes as json if the proxy received it as json, should not have
> > >>> any other effects...
> > >>
> > >> not this one 2/2, just replied here as it was the applied patch
> > > 
> > > Meh, I suppose technically it does, but requires a... broken(?) client
> > > to trigger it, since no API endpoints with arrays which reach this case
> > > existed before?
> > > 
> > 
> > But Dominik writes in a reply to 2/2:
> > 
> > > 
> > > this patch breaks pve-common without the common 1/3 applied
> > > since the gui sends arrays when the api expects a '-list'
> > > and the  api treats '-list' and '-alist' the same 
> > 
> > I.e., this breaks manager, and the workaround for that is in common, so either
> > http-server breaks older manager (i.e., the version before increasing the
> > versioned dependency for libpve-common-perl >= the one containing 1/3 in manager.
> > 
> > Alternatively this could break older libpve-common-perl, enforcing that manager
> > always gets a workable set of packages for it's state.
> 
> Right, I'll add a breaks entry for common.

Actually this has a *depends* on newer common?

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

[Thomas Lamprecht]

Am 07/06/2023 um 14:25 schrieb Wolfgang Bumiller:
> On Wed, Jun 07, 2023 at 02:23:36PM +0200, Wolfgang Bumiller wrote:
>> On Wed, Jun 07, 2023 at 02:14:37PM +0200, Thomas Lamprecht wrote:
>>> Am 07/06/2023 um 14:08 schrieb Wolfgang Bumiller:
>>>> On Wed, Jun 07, 2023 at 01:59:26PM +0200, Thomas Lamprecht wrote:
>>>>> Am 07/06/2023 um 13:49 schrieb Dominik Csapak:
>>>>>>
>>>>>>
>>>>>> On 6/7/23 13:47, Thomas Lamprecht wrote:
>>>>>>> Am 07/06/2023 um 13:23 schrieb Wolfgang Bumiller:
>>>>>>>> applied both & bumped dependency for pve-common
>>>>>>>>
>>>>>>>
>>>>>>> Isn't this breaking older common too?
>>>>>>>
>>>>>>
>>>>>> no it just forwards the parameters to either the daemon or the other nodes as json if the proxy received it as json, should not have
>>>>>> any other effects...
>>>>>
>>>>> not this one 2/2, just replied here as it was the applied patch
>>>>
>>>> Meh, I suppose technically it does, but requires a... broken(?) client
>>>> to trigger it, since no API endpoints with arrays which reach this case
>>>> existed before?
>>>>
>>>
>>> But Dominik writes in a reply to 2/2:
>>>
>>>>
>>>> this patch breaks pve-common without the common 1/3 applied
>>>> since the gui sends arrays when the api expects a '-list'
>>>> and the  api treats '-list' and '-alist' the same 
>>>
>>> I.e., this breaks manager, and the workaround for that is in common, so either
>>> http-server breaks older manager (i.e., the version before increasing the
>>> versioned dependency for libpve-common-perl >= the one containing 1/3 in manager.
>>>
>>> Alternatively this could break older libpve-common-perl, enforcing that manager
>>> always gets a workable set of packages for it's state.
>>
>> Right, I'll add a breaks entry for common.
> 
> Actually this has a *depends* on newer common?
> 

Yes, I read it backwards, seems this is fine – I'll check more calmly first next time,
sorry.

[pve-devel] [PATCH http-server v3 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)

Acked-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
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

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

[Dominik Csapak]

for the record (since i forgot to mention it in the commit message):

this patch breaks pve-common without the common 1/3 applied
since the gui sends arrays when the api expects a '-list'
and the  api treats '-list' and '-alist' the same

with the parameter conversion in common 1/3 this is worked around
for now until we can change the api/gui

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

[Dominik Csapak]

to get rid of the '-alist' format

Acked-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
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] applied: [PATCH guest-common v3 1/1] vzdump: change 'exclude-path' from alist to an array format

[Wolfgang Bumiller]

applied & bumped, updated dependencies & breaks

[pve-devel] [PATCH qemu-server v3 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

Acked-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
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

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

[Wolfgang Bumiller]

applied & bumped common & guest common deps

[pve-devel] applied-series: [PATCH common/http/guest-common/qemu-server v3] schema/config array support

[Wolfgang Bumiller]

applied remaining common patches and added 'breaks' for old qemu-server
& guest-common to it