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

[Thomas Lamprecht]

some additional nits ^^

maybe add a "pct console" tag in the commit message header, allows to
know what/where this does something faster when scrolling past the
commit message a few months down the road, e.g.:

close #1940: pct console: added ability to specify escape sequence

On 10/9/18 11:13 AM, Dominik Csapak wrote:
> 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.",

on top of Dominic's nice observation a more detailed description would be
nice, e.g. something like:

description => "Escape sequence prefix. For example to use <Ctrl+b q> as the escape sequence pass '^b'.",
default => '^a',


>> +        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

you mean instead of the truthy value 1 the string '-1' - I was a bit confused
initially because 1 and -1 are truthy values in perl...

and maybe a comment that states that undef is default, '-1' explicitly 
disables it and '^x' makes it Ctrl x. (for me the most important is to
document what to use to disable it, else one may wonder what the '-1' actually
does).

As said, just nits for the case you send a v3 anyway, apply them where
they make sense to you :-)

> 
> (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 {
>>