[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