[pve-devel] [PATCH common 1/2] remove read_password_func from cli handler

Thomas Lamprecht t.lamprecht at proxmox.com
Tue Jun 12 15:36:07 CEST 2018


On 6/12/18 12:33 PM, Dominik Csapak wrote:
> we have a param_mapping now, with which we can impement that
> 
> e.g. instead of using:
> 
> sub read_password {
>     # read password
> }
> 
> we can do:
> 
> sub param_mapping {
>     my ($name) = @_;
> 
>     my $password_map = ['password', sub{
> 	# read password
>     }, '<password', 1];
> 
>     my $mapping = {
> 	'apicall1' => [$password_map],
> 	'apicall2' => [$password_map],
> 	...
>     };
> 
>     return $mapping->{$name};
> }
> 
> this has the advantage that we can use different subs for different
> api calls (e.g. pveum passwd vs pveum ticket)

looks OK and IMO the better approach as the old read_password_func
handler, few comments inline.

Further this patch needs some coordination on release, i.e., update
of version dependencies in the using packages and a Breaks for them
here. Should I coordinate that part?

> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  src/PVE/CLIHandler.pm  | 18 ++++++++----------
>  src/PVE/JSONSchema.pm  | 15 +--------------
>  src/PVE/RESTHandler.pm | 26 +++++++++++++-------------
>  3 files changed, 22 insertions(+), 37 deletions(-)
> 
> diff --git a/src/PVE/CLIHandler.pm b/src/PVE/CLIHandler.pm
> index 8c911b2..b398f12 100644
> --- a/src/PVE/CLIHandler.pm
> +++ b/src/PVE/CLIHandler.pm
> @@ -127,7 +127,6 @@ sub generate_usage_str {
>      $separator //= '';
>      $indent //= '';
>  
> -    my $read_password_func = $cli_handler_class->can('read_password');
>      my $param_mapping_func = $cli_handler_class->can('param_mapping') ||
>  	$cli_handler_class->can('string_param_file_mapping');
>  
> @@ -149,7 +148,7 @@ sub generate_usage_str {
>  		    $str .= $indent;
>  		    $str .= $class->usage_str($name, "$prefix $cmd", $arg_param,
>  		                              $fixed_param, $format,
> -		                              $read_password_func, $param_mapping_func);
> +		                              $param_mapping_func);
>  		    $oldclass = $class;
>  
>  		} elsif (defined($def->{$cmd}->{alias}) && ($format eq 'asciidoc')) {
> @@ -173,7 +172,7 @@ sub generate_usage_str {
>  
>  	    $str .= $indent;
>  	    $str .= $class->usage_str($name, $prefix, $arg_param, $fixed_param, $format,
> -	                              $read_password_func, $param_mapping_func);
> +	                              $param_mapping_func);
>  	}
>  	return $str;
>      };
> @@ -466,7 +465,7 @@ sub setup_environment {
>  }
>  
>  my $handle_cmd  = sub {
> -    my ($args, $read_password_func, $preparefunc, $param_mapping_func) = @_;
> +    my ($args, $preparefunc, $param_mapping_func) = @_;
>  
>      $cmddef->{help} = [ __PACKAGE__, 'help', ['extra-args'] ];
>  
> @@ -496,13 +495,13 @@ my $handle_cmd  = sub {
>      my ($class, $name, $arg_param, $uri_param, $outsub) = @{$def || []};
>      $abort->("unknown command '$cmd_str'") if !$class;
>  
> -    my $res = $class->cli_handler($cmd_str, $name, $cmd_args, $arg_param, $uri_param, $read_password_func, $param_mapping_func);
> +    my $res = $class->cli_handler($cmd_str, $name, $cmd_args, $arg_param, $uri_param, $param_mapping_func);
>  
>      &$outsub($res) if $outsub;
>  };
>  
>  my $handle_simple_cmd = sub {
> -    my ($args, $read_password_func, $preparefunc, $param_mapping_func) = @_;
> +    my ($args, $preparefunc, $param_mapping_func) = @_;
>  
>      my ($class, $name, $arg_param, $uri_param, $outsub) = @{$cmddef};
>      die "no class specified" if !$class;
> @@ -531,7 +530,7 @@ my $handle_simple_cmd = sub {
>  
>      &$preparefunc() if $preparefunc;
>  
> -    my $res = $class->cli_handler($name, $name, \@ARGV, $arg_param, $uri_param, $read_password_func, $param_mapping_func);
> +    my $res = $class->cli_handler($name, $name, \@ARGV, $arg_param, $uri_param, $param_mapping_func);
>  
>      &$outsub($res) if $outsub;
>  };
> @@ -552,7 +551,6 @@ sub run_cli_handler {
>  
>      my $preparefunc = $params{prepare};
>  
> -    my $read_password_func = $class->can('read_password');
>      my $param_mapping_func = $cli_handler_class->can('param_mapping') ||
>  	$class->can('string_param_file_mapping');
>  
> @@ -564,9 +562,9 @@ sub run_cli_handler {
>      $cmddef = ${"${class}::cmddef"};
>  
>      if (ref($cmddef) eq 'ARRAY') {
> -	&$handle_simple_cmd(\@ARGV, $read_password_func, $preparefunc, $param_mapping_func);
> +	&$handle_simple_cmd(\@ARGV, $preparefunc, $param_mapping_func);
>      } else {
> -	&$handle_cmd(\@ARGV, $read_password_func, $preparefunc, $param_mapping_func);
> +	&$handle_cmd(\@ARGV, $preparefunc, $param_mapping_func);

can we switch those to $handle_cmd->() syntax if we touch those here?

>      }
>  
>      exit 0;
> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
> index f014dc3..1259195 100644
> --- a/src/PVE/JSONSchema.pm
> +++ b/src/PVE/JSONSchema.pm
> @@ -1333,7 +1333,7 @@ sub method_get_child_link {
>  # a way to parse command line parameters, using a 
>  # schema to configure Getopt::Long
>  sub get_options {
> -    my ($schema, $args, $arg_param, $fixed_param, $pwcallback, $param_mapping_hash) = @_;
> +    my ($schema, $args, $arg_param, $fixed_param, $param_mapping_hash) = @_;
>  
>      if (!$schema || !$schema->{properties}) {
>  	raise("too many arguments\n", code => HTTP_BAD_REQUEST)
> @@ -1362,11 +1362,6 @@ sub get_options {
>  	    # optional and call the mapping function afterwards.
>  	    push @getopt, "$prop:s";
>  	    push @interactive, [$prop, $mapping->{func}];
> -	} elsif ($prop eq 'password' && $pwcallback) {
> -	    # we do not accept plain password on input line, instead
> -	    # we turn this into a boolean option and ask for password below
> -	    # using $pwcallback() (for security reasons).
> -	    push @getopt, "$prop";
>  	} elsif ($pd->{type} eq 'boolean') {
>  	    push @getopt, "$prop:s";
>  	} else {
> @@ -1418,14 +1413,6 @@ sub get_options {
>  	}
>      }
>  
> -    if (my $pd = $schema->{properties}->{password}) {
> -	if ($pd->{type} ne 'boolean' && $pwcallback) {
> -	    if ($opts->{password} || !$pd->{optional}) {
> -		$opts->{password} = &$pwcallback(); 
> -	    }
> -	}
> -    }
> -
>      foreach my $entry (@interactive) {
>  	my ($opt, $func) = @$entry;
>  	my $pd = $schema->{properties}->{$opt};
> diff --git a/src/PVE/RESTHandler.pm b/src/PVE/RESTHandler.pm
> index 5e70503..44af354 100644
> --- a/src/PVE/RESTHandler.pm
> +++ b/src/PVE/RESTHandler.pm
> @@ -429,7 +429,7 @@ sub handle {
>  # $style: 'config', 'config-sub', 'arg' or 'fixed'
>  # $mapdef: parameter mapping ({ desc => XXX, func => sub {...} })
>  my $get_property_description = sub {
> -    my ($name, $style, $phash, $format, $hidepw, $mapdef) = @_;
> +    my ($name, $style, $phash, $format, $mapdef) = @_;
>  
>      my $res = '';
>  
> @@ -446,7 +446,7 @@ my $get_property_description = sub {
>  
>      my $type_text = PVE::JSONSchema::schema_get_type_text($phash, $style);
>  
> -    if ($hidepw && $name eq 'password') {
> +    if ($mapdef && $name eq 'password') {

hmm not sure if $hidepw can be just replaced with $mapdef?
Just because there is any mapping does not mean that a
password mapping must be there, or?

>  	$type_text = '';
>      }
>  
> @@ -573,10 +573,9 @@ my $compute_param_mapping_hash = sub {
>  #   'short'    ... command line only (text, one line)
>  #   'full'     ... text, include description
>  #   'asciidoc' ... generate asciidoc for man pages (like 'full')
> -# $hidepw      ... hide password option (use this if you provide a read passwork callback)
>  # $param_mapping_func ... mapping for string parameters to file path parameters
>  sub usage_str {
> -    my ($self, $name, $prefix, $arg_param, $fixed_param, $format, $hidepw, $param_mapping_func) = @_;
> +    my ($self, $name, $prefix, $arg_param, $fixed_param, $format, $param_mapping_func) = @_;
>  
>      $format = 'long' if !$format;
>  
> @@ -609,7 +608,7 @@ sub usage_str {
>      foreach my $k (@$arg_param) {
>  	next if defined($fixed_param->{$k}); # just to be sure
>  	next if !$prop->{$k}; # just to be sure
> -	$argdescr .= &$get_property_description($k, 'fixed', $prop->{$k}, $format, 0);
> +	$argdescr .= &$get_property_description($k, 'fixed', $prop->{$k}, $format);
>      }
>  
>      my $idx_param = {}; # -vlan\d+ -scsi\d+
> @@ -621,7 +620,10 @@ sub usage_str {
>  
>  	my $type_text = $prop->{$k}->{type} || 'string';
>  
> -	next if $hidepw && ($k eq 'password') && !$prop->{$k}->{optional};
> +	my $param_mapping_hash = $compute_param_mapping_hash->(&$param_mapping_func($name))

1. define under postif is possible harmful[1] (I know you did not
introduce this, now is still a good moment to fix this)

maybe better to use a real if for this? something like:

my $param_mapping_hash = {};
if ($param_mapping_func) {
    $param_mapping_hash = $compute_param_mapping_hash->($param_mapping_func->($name));

    next if $param_mapping_hash->{password} && ($k eq 'password') && !$prop->{$k}->{optional};
}

[1]: https://pve.proxmox.com/pipermail/pve-devel/2018-June/032238.html

> +	    if $param_mapping_func;
> +
> +	next if $param_mapping_hash->{password} && ($k eq 'password') && !$prop->{$k}->{optional};
>  
>  	my $base = $k;
>  	if ($k =~ m/^([a-z]+)(\d+)$/) {
> @@ -633,11 +635,9 @@ sub usage_str {
>  	    }
>  	}
>  
> -	my $param_mapping_hash = $compute_param_mapping_hash->(&$param_mapping_func($name))
> -	    if $param_mapping_func;
>  
>  	$opts .= &$get_property_description($base, 'arg', $prop->{$k}, $format,
> -					    $hidepw, $param_mapping_hash->{$k});
> +					    $param_mapping_hash->{$k});
>  
>  	if (!$prop->{$k}->{optional}) {
>  	    $args .= " " if $args;
> @@ -700,7 +700,7 @@ sub dump_properties {
>  	    }
>  	}
>  
> -	$raw .= &$get_property_description($base, $style, $phash, $format, 0);
> +	$raw .= &$get_property_description($base, $style, $phash, $format);
>  
>  	next if $style ne 'config';
>  
> @@ -733,14 +733,14 @@ my $replace_file_names_with_contents = sub {
>  };
>  
>  sub cli_handler {
> -    my ($self, $prefix, $name, $args, $arg_param, $fixed_param, $read_password_func, $param_mapping_func) = @_;
> +    my ($self, $prefix, $name, $args, $arg_param, $fixed_param, $param_mapping_func) = @_;
>  
>      my $info = $self->map_method_by_name($name);
>  
>      my $res;
>      eval {
>  	my $param_mapping_hash = $compute_param_mapping_hash->($param_mapping_func->($name)) if $param_mapping_func;
> -	my $param = PVE::JSONSchema::get_options($info->{parameters}, $args, $arg_param, $fixed_param, $read_password_func, $param_mapping_hash);
> +	my $param = PVE::JSONSchema::get_options($info->{parameters}, $args, $arg_param, $fixed_param, $param_mapping_hash);
>  
>  	if (defined($param_mapping_hash)) {
>  	    &$replace_file_names_with_contents($param, $param_mapping_hash);
> @@ -753,7 +753,7 @@ sub cli_handler {
>  
>  	die $err if !$ec || $ec ne "PVE::Exception" || !$err->is_param_exc();
>  	
> -	$err->{usage} = $self->usage_str($name, $prefix, $arg_param, $fixed_param, 'short', $read_password_func, $param_mapping_func);
> +	$err->{usage} = $self->usage_str($name, $prefix, $arg_param, $fixed_param, 'short',  $param_mapping_func);
>  
>  	die $err;
>      }
> 





More information about the pve-devel mailing list