[pve-devel] [PATCH common v2 1/3] cli: prepare CLIHandler for handling sub-commands
Thomas Lamprecht
t.lamprecht at proxmox.com
Thu Oct 12 10:37:59 CEST 2017
some comments inline, did not yet went over all...
On 10/11/2017 06:58 PM, Philip Abernethy wrote:
> instead of 5 slightly different calls to RESTHandler::usage_str this
> introduces a wrapper function that handles all required cases and is
> capable of resolving sub-commands and aliases.
> Adds a subroutine to print the short help for a command in case no
> subcommand was given.
> Modifies handle_cmd and print_bash_completion to allow for parsing of
> subcommands and aliases.
> ---
changes since v2?
> src/PVE/CLIHandler.pm | 293 ++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 215 insertions(+), 78 deletions(-)
>
> diff --git a/src/PVE/CLIHandler.pm b/src/PVE/CLIHandler.pm
> index e61fa6a..683403b 100644
> --- a/src/PVE/CLIHandler.pm
> +++ b/src/PVE/CLIHandler.pm
> @@ -2,7 +2,6 @@ package PVE::CLIHandler;
>
> use strict;
> use warnings;
> -use Data::Dumper;
>
> use PVE::SafeSyslog;
> use PVE::Exception qw(raise raise_param_exc);
It would be nice to have a comment about the possible structures of $cmddef,
here at the top where it's declared.
I think you got a good overview of the capabilities of this class so this
would be really great, e.g. describe simple vs non-simple commands, aliases,
the new sub commands, ... in a few lines.
> @@ -48,6 +47,85 @@ my $complete_command_names = sub {
> return $res;
> };
>
> +my $generate_usage_str;
> +$generate_usage_str = sub {
> + my ($args) = @_;
> + die "not initialized" if !($cmddef && $exename && $cli_handler_class);
> + die 'format required' if !$args->{format};
the different quotation styles look a bit funny ^^
> +
> + # Set the defaults
> + $args->{sortfunc} //= sub {
> + my ($hash) = @_;
> + return sort keys %$hash;
> + };
> + $args->{cmd} = $cmddef->{$args->{cmd}}->{alias}
> + if (defined($args->{cmd}) && ref($cmddef->{$args->{cmd}}) eq 'HASH' && defined($cmddef->{$args->{cmd}}->{alias}));
I'd put this below in the other `if(defined($args->{cmd}))` branch,
saves you an condition in the if here
> + $args->{base} //= $cmddef;
> + $args->{prefix} //= $exename;
> + if (defined($args->{cmd})) {
> + # If cmd is given, yet base accordingly
> + my @cmds = split(' ', $args->{cmd});
> + $args->{prefix} .= " $args->{cmd}";
> + while (@cmds) {
> + $args->{base} = $args->{base}->{shift @cmds};
> + }
> + }
> + $args->{pwcallback} //= $cli_handler_class->can('read_password');
> + $args->{stringfilemap} //= $cli_handler_class->can('string_param_file_mapping');
> + $args->{sect_sep} //= "";
> + $args->{indent} //= "";
> +
I'd use variables and not always just operate with $args-> as it
makes it a bit harder to read. E.g., something like:
# Set the defaults
my $sortfunc = $args->{sortfunc} // sub { sort keys %{$_[0]}; };
my $base = $args->{base} // $cmddef;
my $prefix = $args->{prefix} // $exename;
my $pwcallback = $args->{pwcallback} // $cli_handler_class->can('read_password');
my $stringfilemap = $args->{stringfilemap} // $cli_handler_class->can('string_param_file_mapping');
my $separator = $args->{sect_sep} // '';
my $indent = $args->{indent} // '';
#
my $cmd = $args->{cmd};
if ($cmd) {
$cmd = $cmddef->{$args->{cmd}}->{alias}
if ref($cmddef->{$cmd}) eq 'HASH' && $cmddef->{$cmd}->{alias};
...
}
> + my $str = "";
> + if (ref($args->{base}) eq 'HASH') {
> + my $oldclass = undef;
> + foreach my $cmd ($args->{sortfunc}->($args->{base})) {
> + if (ref($args->{base}->{$cmd}) eq 'ARRAY') {
> + # $cmd is an array, so it's an actual command
> + my ($class, $name, $arg_param, $fixed_param) = @{$args->{base}->{$cmd}};
> + $str .= $args->{sect_sep} if $oldclass && $oldclass ne $class;
> + $str .= $args->{indent};
> + $str .= $class->usage_str($name, "$args->{prefix} $cmd", $arg_param,
> + $fixed_param, $args->{format}, $args->{pwcallback},
> + $args->{stringfilemap});
> + $oldclass = $class;
> + } elsif (defined($args->{base}->{$cmd}->{alias}) && ($args->{format} eq 'asciidoc')) {
> + # Handle asciidoc separately
> + $str .= "*$args->{prefix} $cmd*\n\nAn alias for '$exename $args->{base}->{$cmd}->{alias}'.\n\n";
> + } else {
> + # $cmd has sub-commands or is an alias
> + next if $args->{base}->{$cmd}->{alias};
> + my $substr .= $generate_usage_str->({
> + format => $args->{format},
> + sortfunc => $args->{sortfunc},
> + base => $args->{base}->{$cmd},
> + prefix => "$args->{prefix} $cmd",
> + pwcallback => $args->{pwcallback},
> + stringfilemap => $args->{stringfilemap},
> + sect_sep => $args->{sect_sep},
> + indent => $args->{indent},
> + });
> + if ($substr) {
> + $substr .= $args->{sect_sep} if $substr !~ /$args->{sect_sep}$args->{sect_sep}$/;
> + $str .= $substr;
> + }
> + }
> + }
> + } else {
> + # Handle simple commands
> + my ($class, $name, $arg_param, $fixed_param) = @{$args->{base} || []};
> +
> + if (!$class) {
> + print_usage_short (\*STDERR, "unknown command '" . join(' ', $args->{cmd}) . "'");
> + exit (-1);
> + }
> +
> + $str .= $args->{indent};
> + $str .= $class->usage_str($name, $args->{prefix}, $arg_param, $fixed_param,
> + $args->{format}, $args->{pwcallback}, $args->{stringfilemap});
> + }
> + return $str;
> +};
> +
> __PACKAGE__->register_method ({
> name => 'help',
> path => 'help',
> @@ -90,18 +168,24 @@ __PACKAGE__->register_method ({
> return undef;
> }
>
> - $cmd = &$expand_command_name($cmddef, $cmd);
> -
> - my ($class, $name, $arg_param, $uri_param) = @{$cmddef->{$cmd} || []};
> -
> - raise_param_exc({ cmd => "no such command '$cmd'"}) if !$class;
> + my $base = $cmddef;
> + my @cmds = split(/ /, $cmd);
> + my @newcmd;
> + while (scalar(@cmds) > 0) {
> + # Auto-complete command
> + last if (ref($base) eq 'ARRAY');
> + push @newcmd, &$expand_command_name($base, shift @cmds);
> + $base = $base->{$newcmd[-1]};
> + }
> + $cmd = join(' ', @newcmd);
>
> - my $pwcallback = $cli_handler_class->can('read_password');
> - my $stringfilemap = $cli_handler_class->can('string_param_file_mapping');
> + my $str = &$generate_usage_str({
> + format => $verbose ? 'full' : 'short',
> + cmd => $cmd,
> + indent => $verbose ? '' : ' ' x 7,
> + });
> + $str =~ s/^\s+//;
>
> - my $str = $class->usage_str($name, "$exename $cmd", $arg_param, $uri_param,
> - $verbose ? 'full' : 'short', $pwcallback,
> - $stringfilemap);
> if ($verbose) {
> print "$str\n";
> } else {
> @@ -113,17 +197,10 @@ __PACKAGE__->register_method ({
> }});
>
> sub print_simple_asciidoc_synopsis {
> - my ($class, $name, $arg_param, $uri_param) = @_;
> -
> die "not initialized" if !$cli_handler_class;
>
> - my $pwcallback = $cli_handler_class->can('read_password');
> - my $stringfilemap = $cli_handler_class->can('string_param_file_mapping');
> -
> - my $synopsis = "*${name}* `help`\n\n";
> -
> - $synopsis .= $class->usage_str($name, $name, $arg_param, $uri_param,
> - 'asciidoc', $pwcallback, $stringfilemap);
> + my $synopsis = "*${exename}* `help`\n\n";
> + $synopsis .= &$generate_usage_str({format => 'asciidoc'});
>
> return $synopsis;
> }
> @@ -132,24 +209,11 @@ sub print_asciidoc_synopsis {
>
> die "not initialized" if !($cmddef && $exename && $cli_handler_class);
>
> - my $pwcallback = $cli_handler_class->can('read_password');
> - my $stringfilemap = $cli_handler_class->can('string_param_file_mapping');
> -
> my $synopsis = "";
>
> $synopsis .= "*${exename}* `<COMMAND> [ARGS] [OPTIONS]`\n\n";
>
> - my $oldclass;
> - foreach my $cmd (sort keys %$cmddef) {
> - my ($class, $name, $arg_param, $uri_param) = @{$cmddef->{$cmd}};
> - my $str = $class->usage_str($name, "$exename $cmd", $arg_param,
> - $uri_param, 'asciidoc', $pwcallback,
> - $stringfilemap);
> - $synopsis .= "\n" if $oldclass && $oldclass ne $class;
> -
> - $synopsis .= "$str\n\n";
> - $oldclass = $class;
> - }
> + $synopsis .= &$generate_usage_str({format => 'asciidoc'});
>
> $synopsis .= "\n";
>
> @@ -160,21 +224,11 @@ sub print_usage_verbose {
>
> die "not initialized" if !($cmddef && $exename && $cli_handler_class);
>
> - my $pwcallback = $cli_handler_class->can('read_password');
> - my $stringfilemap = $cli_handler_class->can('string_param_file_mapping');
> -
> print "USAGE: $exename <COMMAND> [ARGS] [OPTIONS]\n\n";
>
> - foreach my $cmd (sort keys %$cmddef) {
> - my ($class, $name, $arg_param, $uri_param) = @{$cmddef->{$cmd}};
> - my $str = $class->usage_str($name, "$exename $cmd", $arg_param, $uri_param,
> - 'full', $pwcallback, $stringfilemap);
> - print "$str\n\n";
> - }
> -}
> + my $str = &$generate_usage_str({format => 'full'});
>
> -sub sorted_commands {
> - return sort { ($cmddef->{$a}->[0] cmp $cmddef->{$b}->[0]) || ($a cmp $b)} keys %$cmddef;
> + print "$str\n";
> }
>
> sub print_usage_short {
> @@ -182,22 +236,54 @@ sub print_usage_short {
>
> die "not initialized" if !($cmddef && $exename && $cli_handler_class);
>
> - my $pwcallback = $cli_handler_class->can('read_password');
> - my $stringfilemap = $cli_handler_class->can('string_param_file_mapping');
> -
> print $fd "ERROR: $msg\n" if $msg;
> print $fd "USAGE: $exename <COMMAND> [ARGS] [OPTIONS]\n";
>
> - my $oldclass;
> - foreach my $cmd (sorted_commands()) {
> - my ($class, $name, $arg_param, $uri_param) = @{$cmddef->{$cmd}};
> - my $str = $class->usage_str($name, "$exename $cmd", $arg_param, $uri_param, 'short', $pwcallback, $stringfilemap);
> - print $fd "\n" if $oldclass && $oldclass ne $class;
> - print $fd " $str";
> - $oldclass = $class;
> - }
> + print &$generate_usage_str({format => 'short', sect_sep => "\n", sortfunc =>
> + sub {
> + my ($hash) = @_;
> + return sort {
> + if ((ref($hash->{$a}) eq 'ARRAY' && ref($hash->{$b}) eq 'ARRAY') &&
> + ($hash->{$a}->[0] ne $hash->{$b}->[0])) {
> + # If $a and $b are both arrays (commands) and the commands are not in
> + # the same class, order their classes alphabetically
> + return $hash->{$a}->[0] cmp $hash->{$b}->[0];
> + } elsif (ref($hash->{$a}) eq 'ARRAY' xor ref($hash->{$b}) eq 'ARRAY') {
> + # If one is an array (command) and one is a hash (has subcommands),
> + # sort commands behind sub.commands
> + return ref($hash->{$b}) eq 'ARRAY' ? -1 : 1;
> + } else {
> + # If $a and $b are both commands of the same class or both sub-commands,
> + # sort alphabetically
> + return $a cmp $b;
> + }
> + } keys %$hash;
> + }, indent => ' ' x 7});
> }
>
> +my $print_help_short = sub {
> + my ($fd, $cmd, $msg) = @_;
> +
> + die "not initialized" if !($cmddef);
> +
> + print $fd "ERROR: $msg\n" if $msg;
> +
> + my $base = $cmddef;
> + while (scalar(@$cmd) > 1) {
> + $base = $base->{shift @$cmd};
> + }
> +
> + my $str = &$generate_usage_str({
> + format => 'short',
> + base => $base,
> + cmd => $cmd->[0],
> + indent => ' ' x 7,
> + });
> + $str =~ s/^\s+//;
> +
> + print "USAGE: $str\n";
> +};
> +
> my $print_bash_completion = sub {
> my ($cmddef, $simple_cmd, $bash_command, $cur, $prev) = @_;
>
> @@ -225,17 +311,40 @@ my $print_bash_completion = sub {
> };
>
> my $cmd;
> + my $def = $cmddef;
> + my $cmd_depth = 0;
> + if (scalar(@$args) > 1) {
> + for my $i (1 .. $#$args) {
> + last if (ref($def) eq 'ARRAY');
> + if (@$args[$i] ne $cur && exists $def->{@$args[$i]}) {
> + # Move def to proper sub-command-def
> + # Don't try yet-to-complete commands
> + # exists… prevents auto-vivification
> + $def = $def->{@$args[$i]};
> + $cmd_depth++;
> + }
> + }
> + }
> if ($simple_cmd) {
> $cmd = $simple_cmd;
> + $def = $def->{$simple_cmd};
> } else {
> - if ($pos == 0) {
> - &$print_result(keys %$cmddef);
> - return;
> + if (ref($def) eq 'HASH') {
> + if (exists $def->{alias}) {
> + # Move def to aliased command
> + my $newdef = $cmddef;
> + foreach my $subcmd (split(/ /, $def->{alias})) {
> + $newdef = $newdef->{$subcmd};
> + }
> + $def = $newdef;
> + } else {
> + &$print_result(keys %$def);
> + return;
> + }
> }
> - $cmd = $args->[1];
> + $cmd = @$args[-1];
> }
>
> - my $def = $cmddef->{$cmd};
> return if !$def;
>
> print STDERR "CMDLINE1:$pos:$cmdline\n" if $debug;
> @@ -251,12 +360,11 @@ my $print_bash_completion = sub {
> map { $skip_param->{$_} = 1; } @$arg_param;
> map { $skip_param->{$_} = 1; } keys %$uri_param;
>
> - my $fpcount = scalar(@$arg_param);
> + my $fpcount = scalar(@$arg_param) + $cmd_depth - 1;
>
> my $info = $class->map_method_by_name($name);
>
> - my $schema = $info->{parameters};
> - my $prop = $schema->{properties};
> + my $prop = $info->{parameters}->{properties};
>
> my $print_parameter_completion = sub {
> my ($pname) = @_;
> @@ -277,7 +385,7 @@ my $print_bash_completion = sub {
> # positional arguments
> $pos += 1 if $simple_cmd;
> if ($fpcount && $pos <= $fpcount) {
> - my $pname = $arg_param->[$pos -1];
> + my $pname = $arg_param->[$pos - $cmd_depth];
> &$print_parameter_completion($pname);
> return;
> }
> @@ -375,12 +483,11 @@ sub generate_asciidoc_synopsis {
>
> no strict 'refs';
> my $def = ${"${class}::cmddef"};
> + $cmddef = $def;
>
> if (ref($def) eq 'ARRAY') {
> print_simple_asciidoc_synopsis(@$def);
> } else {
> - $cmddef = $def;
> -
> $cmddef->{help} = [ __PACKAGE__, 'help', ['cmd'] ];
>
> print_asciidoc_synopsis();
> @@ -405,33 +512,62 @@ my $handle_cmd = sub {
> # call verifyapi before setup_environment(), because we do not want to
> # execute any real code in this case
>
> - if (!$cmd) {
> + if (!defined($cmd->[0])) {
> print_usage_short (\*STDERR, "no command specified");
> exit (-1);
> - } elsif ($cmd eq 'verifyapi') {
> + } elsif ($cmd->[0] eq 'verifyapi') {
> PVE::RESTHandler::validate_method_schemas();
> return;
> }
>
> $cli_handler_class->setup_environment();
>
> - if ($cmd eq 'bashcomplete') {
> + if ($cmd->[0] eq 'bashcomplete') {
> &$print_bash_completion($cmddef, 0, @$args);
> return;
> }
>
> &$preparefunc() if $preparefunc;
>
> - $cmd = &$expand_command_name($cmddef, $cmd);
@cmd is $ARGV[0]
$args is @ARGV[1:] (all but the first element)
here...
> + unshift @$args, shift @$cmd;
and now @cmd is empty and $args is again @ARGV,
this seems really wrong/weird
pass cmd as a normal scalar ref (string) and declare a new array here
if you need it.
Or just pass \@ARGV to this method, it has already all the information
but please do not split it to re-combine directly here again.
as alternative for above you could also always:
foreach my $arg ($cmd, @$args) {
... #
}
> + my $base = $def;
if we do the cleanup patch before this I'd use $def for the "$cmddef" iterator
base is a bit ambiguos and $expand_command_name uses also $def.
> + while (scalar(@$args) > 0) {
please use foreach if you loop just over all array elements.
> + last if (ref($base) eq 'ARRAY');
> + # Auto-complete commands
> + push @$cmd, &$expand_command_name($base, shift @$args);
> + $base = $base->{$cmd->[-1]};
With a foreach and a sane value $iterator this should get easier to read
> + if (ref($base) eq 'HASH' && defined($base->{alias})) {
> + # If command is an alias, reset $base and move to aliased command
Sounds like it could be done in recursion?
> + my @alias = split(/ /, $base->{alias})> + $base = $def;
> + undef(@$cmd);
> + while (@alias) {
> + unshift @$args, pop @alias;
> + }
If this would be used here it could be done with:
unshift @$args, split(/ +/, $base->{alias}); # assuming that $args is @ARGV[1:] not @ARGV[0:]
$base = $def; $cmd = [];
And you do not check for potentially alias loops.
I'd just allow one level of alias for now, everything else
is currently not needed.
> + }
> + }
> +
> + if (ref($base) eq 'HASH') {
> + &$print_help_short (\*STDERR, $cmd, "incomplete command '" . join(' ', @$cmd) . "'");
> + exit (-1);
> + }
>
> - my ($class, $name, $arg_param, $uri_param, $outsub) = @{$cmddef->{$cmd} || []};
> + my ($class, $name, $arg_param, $uri_param, $outsub) = @{$base || []};
>
> if (!$class) {
> - print_usage_short (\*STDERR, "unknown command '$cmd'");
> + print_usage_short (\*STDERR, "unknown command '" . join(' ', @$cmd) . "'");
> exit (-1);
> + } elsif ($name eq 'help') {
> + # Find command help is wanted for
> + my @help_cmd;
> + while (@ARGV) {
> + last if ($ARGV[0] =~ /^-/);
what if `qm help create 100`, the /^-/ heuristics fails here.
Just consume the next word and check if ther may be a (sub) command
for it in a loop, on the first failure you know that there may not
be any help for this and abort?
> + push @help_cmd, shift @ARGV;
> + }
> + unshift @ARGV, join(' ', @help_cmd);
> }
>
> - my $prefix = "$exename $cmd";
> + my $prefix = "$exename " . join(' ', @$cmd);
> my $res = $class->cli_handler($prefix, $name, \@ARGV, $arg_param, $uri_param, $pwcallback, $stringfilemap);
>
> &$outsub($res) if $outsub;
> @@ -446,7 +582,7 @@ my $handle_simple_cmd = sub {
> if (scalar(@$args) >= 1) {
> if ($args->[0] eq 'help') {
> my $str = "USAGE: $name help\n";
> - $str .= $class->usage_str($name, $name, $arg_param, $uri_param, 'long', $pwcallback, $stringfilemap);
> + $str .= &$generate_usage_str({format => 'long'});
> print STDERR "$str\n\n";
> return;
> } elsif ($args->[0] eq 'verifyapi') {
> @@ -508,13 +644,14 @@ sub run_cli_handler {
>
> no strict 'refs';
> my $def = ${"${class}::cmddef"};
> + $cmddef = $def;
>
> if (ref($def) eq 'ARRAY') {
> &$handle_simple_cmd($def, \@ARGV, $pwcallback, $preparefunc, $stringfilemap);
> } else {
> $cmddef = $def;
> - my $cmd = shift @ARGV;
> - &$handle_cmd($cmddef, $exename, $cmd, \@ARGV, $pwcallback, $preparefunc, $stringfilemap);
> + my @cmd = shift @ARGV;
why an array? after this cmd is just the first parameter of the CLI call,
e.g., with:
# pveum foo bar
cmd is here then 'foo', you then pass a reference to it down to
$handle_cmd->()
Why not just keep the
my $cmd = shift @ARGV;
?
Then you would not need to change all $cmd calls in the $handle_cmd sub to $cmd->[0];
You remove this all in the next cleanup patch, would it make sense
to move (parts) of the cleanup in front of this patch?
If its already cleaned up the new sub command paths may be a bit easier to
review, especially as things get only added once and not added then removed.
> + &$handle_cmd($cmddef, $exename, \@cmd, \@ARGV, $pwcallback, $preparefunc, $stringfilemap);
> }
>
> exit 0;
>
More information about the pve-devel
mailing list