* [pve-devel] [PATCH common v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config
2023-06-06 8:39 [pve-devel] [PATCH common/http/guest-common/qemu-server v2] schema/config array support Dominik Csapak
@ 2023-06-06 8:39 ` Dominik Csapak
2023-06-06 9:12 ` Thomas Lamprecht
2023-06-06 8:39 ` [pve-devel] [PATCH common v2 2/3] section config: implement array support Dominik Csapak
` (5 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Dominik Csapak @ 2023-06-06 8:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [pve-devel] [PATCH common v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config
2023-06-06 8:39 ` [pve-devel] [PATCH common v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config Dominik Csapak
@ 2023-06-06 9:12 ` Thomas Lamprecht
2023-06-06 9:41 ` Dominik Csapak
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Lamprecht @ 2023-06-06 9:12 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [pve-devel] [PATCH common v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config
2023-06-06 9:12 ` Thomas Lamprecht
@ 2023-06-06 9:41 ` Dominik Csapak
2023-06-06 10:45 ` Thomas Lamprecht
0 siblings, 1 reply; 14+ messages in thread
From: Dominik Csapak @ 2023-06-06 9:41 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [pve-devel] [PATCH common v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config
2023-06-06 9:41 ` Dominik Csapak
@ 2023-06-06 10:45 ` Thomas Lamprecht
2023-06-06 11:19 ` Dominik Csapak
2023-06-06 12:05 ` Wolfgang Bumiller
0 siblings, 2 replies; 14+ messages in thread
From: Thomas Lamprecht @ 2023-06-06 10:45 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
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).
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [pve-devel] [PATCH common v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config
2023-06-06 10:45 ` Thomas Lamprecht
@ 2023-06-06 11:19 ` Dominik Csapak
2023-06-06 11:24 ` Thomas Lamprecht
2023-06-06 12:05 ` Wolfgang Bumiller
1 sibling, 1 reply; 14+ messages in thread
From: Dominik Csapak @ 2023-06-06 11:19 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion
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...
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [pve-devel] [PATCH common v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config
2023-06-06 11:19 ` Dominik Csapak
@ 2023-06-06 11:24 ` Thomas Lamprecht
0 siblings, 0 replies; 14+ messages in thread
From: Thomas Lamprecht @ 2023-06-06 11:24 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pve-devel] [PATCH common v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config
2023-06-06 10:45 ` Thomas Lamprecht
2023-06-06 11:19 ` Dominik Csapak
@ 2023-06-06 12:05 ` Wolfgang Bumiller
1 sibling, 0 replies; 14+ messages in thread
From: Wolfgang Bumiller @ 2023-06-06 12:05 UTC (permalink / raw)
To: Thomas Lamprecht; +Cc: Proxmox VE development discussion, Dominik Csapak
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 ;-)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [pve-devel] [PATCH common v2 2/3] section config: implement array support
2023-06-06 8:39 [pve-devel] [PATCH common/http/guest-common/qemu-server v2] schema/config array support Dominik Csapak
2023-06-06 8:39 ` [pve-devel] [PATCH common v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config Dominik Csapak
@ 2023-06-06 8:39 ` Dominik Csapak
2023-06-06 8:39 ` [pve-devel] [PATCH common v2 3/3] JSONSchema: disable '-alist' format Dominik Csapak
` (4 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Dominik Csapak @ 2023-06-06 8:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [pve-devel] [PATCH common v2 3/3] JSONSchema: disable '-alist' format
2023-06-06 8:39 [pve-devel] [PATCH common/http/guest-common/qemu-server v2] schema/config array support Dominik Csapak
2023-06-06 8:39 ` [pve-devel] [PATCH common v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config Dominik Csapak
2023-06-06 8:39 ` [pve-devel] [PATCH common v2 2/3] section config: implement array support Dominik Csapak
@ 2023-06-06 8:39 ` Dominik Csapak
2023-06-06 8:39 ` [pve-devel] [PATCH http-server v2 1/2] proxy request: forward json content type and parameters Dominik Csapak
` (3 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Dominik Csapak @ 2023-06-06 8:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [pve-devel] [PATCH http-server v2 1/2] proxy request: forward json content type and parameters
2023-06-06 8:39 [pve-devel] [PATCH common/http/guest-common/qemu-server v2] schema/config array support Dominik Csapak
` (2 preceding siblings ...)
2023-06-06 8:39 ` [pve-devel] [PATCH common v2 3/3] JSONSchema: disable '-alist' format Dominik Csapak
@ 2023-06-06 8:39 ` Dominik Csapak
2023-06-06 8:39 ` [pve-devel] [PATCH http-server v2 2/2] use proper arrays for array parameter Dominik Csapak
` (2 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Dominik Csapak @ 2023-06-06 8:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [pve-devel] [PATCH http-server v2 2/2] use proper arrays for array parameter
2023-06-06 8:39 [pve-devel] [PATCH common/http/guest-common/qemu-server v2] schema/config array support Dominik Csapak
` (3 preceding siblings ...)
2023-06-06 8:39 ` [pve-devel] [PATCH http-server v2 1/2] proxy request: forward json content type and parameters Dominik Csapak
@ 2023-06-06 8:39 ` Dominik Csapak
2023-06-06 8:39 ` [pve-devel] [PATCH guest-common v2 1/1] vzdump: change 'exclude-path' from alist to an array format Dominik Csapak
2023-06-06 8:39 ` [pve-devel] [PATCH qemu-server v2 1/1] api: switch agent api call to 'array' type Dominik Csapak
6 siblings, 0 replies; 14+ messages in thread
From: Dominik Csapak @ 2023-06-06 8:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [pve-devel] [PATCH guest-common v2 1/1] vzdump: change 'exclude-path' from alist to an array format
2023-06-06 8:39 [pve-devel] [PATCH common/http/guest-common/qemu-server v2] schema/config array support Dominik Csapak
` (4 preceding siblings ...)
2023-06-06 8:39 ` [pve-devel] [PATCH http-server v2 2/2] use proper arrays for array parameter Dominik Csapak
@ 2023-06-06 8:39 ` Dominik Csapak
2023-06-06 8:39 ` [pve-devel] [PATCH qemu-server v2 1/1] api: switch agent api call to 'array' type Dominik Csapak
6 siblings, 0 replies; 14+ messages in thread
From: Dominik Csapak @ 2023-06-06 8:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [pve-devel] [PATCH qemu-server v2 1/1] api: switch agent api call to 'array' type
2023-06-06 8:39 [pve-devel] [PATCH common/http/guest-common/qemu-server v2] schema/config array support Dominik Csapak
` (5 preceding siblings ...)
2023-06-06 8:39 ` [pve-devel] [PATCH guest-common v2 1/1] vzdump: change 'exclude-path' from alist to an array format Dominik Csapak
@ 2023-06-06 8:39 ` Dominik Csapak
6 siblings, 0 replies; 14+ messages in thread
From: Dominik Csapak @ 2023-06-06 8:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread