[pve-devel] [PATCH pve-manager] pvesh: fix bug #1942 - add standard options conditional

[David Limbeck]

I'd prefer to have at least a warning if there's a conflict and the 
standard output options are not added instead of silently dismissing all 
of them.

As a quick workaround to get pvesh backups working again it's fine, but 
we really should work on a better solution/redesign.

Some comments on the code inline.

On 10/10/18 11:36 AM, Dietmar Maurer wrote:
> Do not add/extract standard options if the method itself defined properties
> using the same names (like 'quiet').
>
> Signed-off-by: Dietmar Maurer <dietmar at proxmox.com>
> ---
>   PVE/CLI/pvesh.pm | 31 +++++++++++++++++++++++++------
>   1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/PVE/CLI/pvesh.pm b/PVE/CLI/pvesh.pm
> index ccfb5c20..03222445 100755
> --- a/PVE/CLI/pvesh.pm
> +++ b/PVE/CLI/pvesh.pm
> @@ -263,6 +263,24 @@ $path_properties->{noproxy} = {
>       optional => 1,
>   };
>   
> +my $extract_std_options = 1;
> +
> +my $cond_add_standard_output_properties = sub {
> +    my ($props) = @_;
> +
> +    my $optlist = [];
$optlist is never used
> +    foreach my $opt (keys %$PVE::RESTHandler::standard_output_options) {
> +	if (defined($props->{$opt})) {
maybe add a simple warn "conflicting option $opt: ignoring any standard 
output options"; or something like that? this shouldn't be silent at all
> +	    $extract_std_options = 0;
> +	    return $props;
> +	}
> +    }
> +
> +    return $props if $props->{quiet};
this already happens in the loop above, so unnecessary. should quiet be 
removed from the standard output options we no longer have a conflict 
and can add the standard output options, why return here instead of 
adding them then?
> +
> +    return PVE::RESTHandler::add_standard_output_properties($props);
> +};
> +
>   sub call_api_method {
>       my ($cmd, $param) = @_;
>   
> @@ -271,7 +289,8 @@ sub call_api_method {
>       my $path = PVE::Tools::extract_param($param, 'api_path');
>       die "missing API path\n" if !defined($path);
>   
> -    my $stdopts =  PVE::RESTHandler::extract_standard_output_properties($param);
> +    my $stdopts =  $extract_std_options ?
> +	PVE::RESTHandler::extract_standard_output_properties($param) : {};
>   
>       $opt_nooutput = 1 if $stdopts->{quiet};
>   
> @@ -305,7 +324,7 @@ __PACKAGE__->register_method ({
>       description => "List child objects on <api_path>.",
>       parameters => {
>   	additionalProperties => 0,
> -	properties => PVE::RESTHandler::add_standard_output_properties($path_properties),
> +	properties => $cond_add_standard_output_properties->($path_properties),
>       },
>       returns => { type => 'null' },
>       code => sub {
> @@ -361,7 +380,7 @@ __PACKAGE__->register_method ({
>       description => "Call API GET on <api_path>.",
>       parameters => {
>   	additionalProperties => 0,
> -	properties => PVE::RESTHandler::add_standard_output_properties($path_properties),
> +	properties => $cond_add_standard_output_properties->($path_properties),
>       },
>       returns => { type => 'null' },
>       code => sub {
> @@ -379,7 +398,7 @@ __PACKAGE__->register_method ({
>       description => "Call API PUT on <api_path>.",
>       parameters => {
>   	additionalProperties => 0,
> -	properties => PVE::RESTHandler::add_standard_output_properties($path_properties),
> +	properties => $cond_add_standard_output_properties->($path_properties),
>       },
>       returns => { type => 'null' },
>       code => sub {
> @@ -397,7 +416,7 @@ __PACKAGE__->register_method ({
>       description => "Call API POST on <api_path>.",
>       parameters => {
>   	additionalProperties => 0,
> -	properties => PVE::RESTHandler::add_standard_output_properties($path_properties),
> +	properties => $cond_add_standard_output_properties->($path_properties),
>       },
>       returns => { type => 'null' },
>       code => sub {
> @@ -415,7 +434,7 @@ __PACKAGE__->register_method ({
>       description => "Call API DELETE on <api_path>.",
>       parameters => {
>   	additionalProperties => 0,
> -	properties => PVE::RESTHandler::add_standard_output_properties($path_properties),
> +	properties => $cond_add_standard_output_properties->($path_properties),
>       },
>       returns => { type => 'null' },
>       code => sub {