[pve-devel] [PATCH v2 container] close #1940: added ability to specify escape sequence

[Dominik Csapak]

comments inline, rest looks good

On 10/9/18 10:47 AM, Tim Marx wrote:
> moved regex into property definition to make use of unified api logic
> removed inline/whitespace
> 
> Signed-off-by: Tim Marx <t.marx at proxmox.com>
> ---
>   src/PVE/CLI/pct.pm | 8 +++++++-
>   src/PVE/LXC.pm     | 6 +++---
>   2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
> index 6296d6f..5ac821e 100755
> --- a/src/PVE/CLI/pct.pm
> +++ b/src/PVE/CLI/pct.pm
> @@ -119,6 +119,12 @@ __PACKAGE__->register_method ({
>       	additionalProperties => 0,
>   	properties => {
>   	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid_running }),
> +	    escape => {
> +		description => "Escape character.",
> +		type => 'string',
> +		pattern => '\^?([a-z])',

the matching group () is not necessary here

> +		optional => 1,
> +	    },
>   	},
>       },
>       returns => { type => 'null' },
> @@ -129,7 +135,7 @@ __PACKAGE__->register_method ({
>   	# test if container exists on this node
>   	my $conf = PVE::LXC::Config->load_config($param->{vmid});
>   
> -	my $cmd = PVE::LXC::get_console_command($param->{vmid}, $conf);
> +	my $cmd = PVE::LXC::get_console_command($param->{vmid}, $conf, $param->{escape});
>   	exec(@$cmd);
>       }});
>   
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 89f289e..09960a8 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -664,17 +664,17 @@ sub verify_searchdomain_list {
>   }
>   
>   sub get_console_command {
> -    my ($vmid, $conf, $noescapechar) = @_;
> +    my ($vmid, $conf, $escapechar) = @_;

since you change the signature of the sub, it would be good to check
if it is used elsewhere

a quick grep across our repos gives me:

pve-container/src/PVE/API2/LXC.pm
736:    my $concmd = PVE::LXC::get_console_command($vmid, $conf, 1);
839:    my $concmd = PVE::LXC::get_console_command($vmid, $conf, 1);
951:    my $concmd = PVE::LXC::get_console_command($vmid, $conf);

so it would be good to change the instances of '1' to '-1' to get
the same behaviour as we had before

(note that in both cases the escape key does not work,
but i think this more due to the fact that lxc parses
that argument wrong)

rest looks good thanks :)


>   
>       my $cmode = PVE::LXC::Config->get_cmode($conf);
>   
>       my $cmd = [];
>       if ($cmode eq 'console') {
>   	push @$cmd, 'lxc-console', '-n',  $vmid, '-t', 0;
> -	push @$cmd, '-e', -1 if $noescapechar;
> +	push @$cmd, '-e', $escapechar if $escapechar;
>       } elsif ($cmode eq 'tty') {
>   	push @$cmd, 'lxc-console', '-n',  $vmid;
> -	push @$cmd, '-e', -1 if $noescapechar;
> +	push @$cmd, '-e', $escapechar if $escapechar;
>       } elsif ($cmode eq 'shell') {
>   	push @$cmd, 'lxc-attach', '--clear-env', '-n', $vmid;
>       } else {
>