[pve-devel] [PATCH common 1/2] remove read_password_func from cli handler
Dominik Csapak
d.csapak at proxmox.com
Wed Jun 13 09:23:47 CEST 2018
On 06/12/2018 03:36 PM, Thomas Lamprecht wrote:
> 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.
thanks for the review
>
> 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?
if you offer it so nicely, yes please do ;)
>
>>
>> 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?
yes, i send a v2.
>
>> }
>>
>> 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?
mapdef is here strictly for the given parameter, not the whole parameter
mapping hash, so if we have a mapping for 'password' then this is true
when processing the 'password' parameter and not anywhere else
(otoh, when we have no passord mapping, this will never be true)
looking again at the code, the whole check is not really necessary,
since directly below we have a:
if (mapdef && parameter-type eq string)
text = mapdef->description
(not real perl syntax i know)
which would override it anyway
instead i would drop the eq 'password' case
and let the parameter mapping handle this
(e.g. giving an empty description in the parameter mapping)
>
>> $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
yes, will be fixed in v2
>
>> + 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;
>> }
>>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
More information about the pve-devel
mailing list