[pve-devel] applied: [PATCH pve-client 2/2] lxc enter: simplify code and cleanups

[Wolfgang Bumiller]

applied, but two comments inline...

On Wed, Jun 06, 2018 at 11:30:13AM +0200, Dietmar Maurer wrote:
> - print error messages after reseting the terminal
> - catch signals
> 
> Signed-off-by: Dietmar Maurer <dietmar at proxmox.com>
> ---
>  PVE/APIClient/Commands/lxc.pm | 152 +++++++++++++++++-------------------------
>  1 file changed, 63 insertions(+), 89 deletions(-)
> 
> diff --git a/PVE/APIClient/Commands/lxc.pm b/PVE/APIClient/Commands/lxc.pm
> index df72625..81dfd3f 100644
> --- a/PVE/APIClient/Commands/lxc.pm
> +++ b/PVE/APIClient/Commands/lxc.pm
> @@ -128,35 +128,6 @@ my $parse_web_socket_frame = sub  {
>      return ($payload, $req_close);
>  };
>  
> -my $client_exit = sub {
> -    my ($select, $web_socket, $old_termios) = @_;
> -
> -    foreach my $fh ($select->handles) {
> -	$select->remove($fh);
> -
> -	if ($fh == $web_socket) {
> -	    if ($fh->connected) {
> -
> -		# close connection
> -		# Opcode, mask, statuscode
> -		my $msg = "\x88" . pack('N', 0) . pack('n', 0);
> -		$fh->syswrite($msg);
> -		close($fh);
> -	    }
> -	}
> -
> -    }
> -
> -    # switch back to blocking mode (else later shell commands will fail).
> -    STDIN->blocking(1);
> -
> -    #
> -    # Reset the terminal parameters.
> -    #
> -    print "\e[24H\r\n";
> -    PVE::PTY::tcsetattr(*STDIN, $old_termios);
> -};
> -
>  __PACKAGE__->register_method ({
>      name => 'enter',
>      path => 'enter',
> @@ -251,87 +222,90 @@ __PACKAGE__->register_method ({
>  	# Set STDIN to "raw -echo" mode
>  	my $old_termios = PVE::PTY::tcgetattr(*STDIN);
>  	my $raw_termios = {%$old_termios};
> -	PVE::PTY::cfmakeraw($raw_termios);
> -	PVE::PTY::tcsetattr(*STDIN, $raw_termios);
> -
> -	# And set it to non-blocking so we can every char with IO::Select.
> -	STDIN->blocking(0);
>  
>  	my $select = IO::Select->new;
>  
> -	$web_socket->blocking(0);
> -	$select->add($web_socket);
> -	$select->add(fileno(STDIN));
> +	eval {
> +	    $SIG{TERM} = $SIG{INT} = $SIG{KILL} = sub { die "received interrupt\n"; };
>  
> -	my @messages;
> -	my $ctrl_a_pressed_before = 0;
> -	my $next_ping = time() + 3;
> +	    PVE::PTY::cfmakeraw($raw_termios);
> +	    PVE::PTY::tcsetattr(*STDIN, $raw_termios);
>  
> -	eval {
> -	    while (1) {
> -		# Ping server every 3 seconds.
> -		my $now = time();
> -		if ($now >= $next_ping) {
> -		    push(@messages, $create_websockt_frame->("2"));
> -		    $next_ping = $now + 3;
> -		}
> +	    # And set it to non-blocking so we can every char with IO::Select.
> +	    STDIN->blocking(0);
>  
> -		# Write
> -		foreach my $fh ($select->can_write(0.5)) {
> -		    if ($fh == $web_socket and my $msg = shift @messages) {
> -			$fh->syswrite($msg, length($msg));
> -		    }
> -		}
> +	    $web_socket->blocking(1);

AFAIK this shouldn't be required, it's never set to non-blocking anymore
from what I can tell and this mode is the default anyway.

> +	    $select->add($web_socket);
> +	    my $input_fh = fileno(STDIN);
> +	    $select->add($input_fh);

This could have just been `$select->add(\*STDIN);`, no? ;-)

>  
> -		# Read
> -		foreach my $fh ($select->can_read(0.5)) {
> +	    my $ctrl_a_pressed_before = 0;
>  
> -		    # From Web Socket
> -		    if ($fh == $web_socket) {
> -			# Read from WebSocket
> -			my $nr = $wb_socket_read_available_bytes->();
> -			my ($payload, $req_close) = $parse_web_socket_frame->(\$wsbuf);
> +	    while (1) {
> +		while(my @ready = $select->can_read(3)) {
> +		    foreach my $fh (@ready) {
> +
> +			if ($fh == $web_socket) {
> +			    # Read from WebSocket
> +
> +			    my $nr = $wb_socket_read_available_bytes->();
> +			    if (!defined($nr)) {
> +				die "web socket read error $!\n";
> +			    } elsif ($nr == 0) {
> +				return; # EOF
> +			    } else {
> +				my ($payload, $req_close) = $parse_web_socket_frame->(\$wsbuf);
> +				if ($payload) {
> +				    syswrite(\*STDOUT, $payload);
> +				}
> +				return if $req_close;
> +			    }
>  
> -			if ($payload ne "OK") {
> -			    syswrite(\*STDOUT, $payload, length($payload));
> -			}
> -		    }
> +			} elsif ($fh == $input_fh) {
> +			    # Read from STDIN
>  
> -		    # From STDIN
> -		    elsif ($fh == fileno(STDIN)) {
> +			    my $nr = read(\*STDIN, my $buff, 4096);
> +			    return if !$nr; # EOF or error
>  
> -			# Read from STDIN
> -			my $nr = read(\*STDIN, my $buff, 4096);
> -			if (!$nr) {
> -			    next;
> -			}
> +			    my $char = ord($buff);
>  
> -			my $char = ord($buff);
> +			    # check for CTRL-a-q
> +			    return if $ctrl_a_pressed_before == 1 && $char == hex("0x71");
>  
> -			if ($ctrl_a_pressed_before == 1 && $char == hex("0x71")) {
> -			    $client_exit->($select, $web_socket, $old_termios);
> -			    return;
> -			}
> +			    $ctrl_a_pressed_before = ($char == hex("0x01") && $ctrl_a_pressed_before == 0) ? 1 : 0;
>  
> -			if ($char == hex("0x01")) {
> -			    if ($ctrl_a_pressed_before == 0) {
> -				$ctrl_a_pressed_before = 1;
> -			    }
> -			}
> -			else {
> -			    $ctrl_a_pressed_before = 0;
> +			    my $frame = $create_websockt_frame->("0:" . $nr . ":" . $buff);
> +			    syswrite($web_socket, $frame);
>  			}
> -
> -			push(@messages, $create_websockt_frame->("0:" . $nr . ":" . $buff));
>  		    }
>  		}
> +		# got timeout
> +		syswrite($web_socket, $create_websockt_frame->("2")); # ping server to keep connection alive
> +	    }
> +	};
> +	my $err = $@;
> +
> +	eval {  # cleanup
> +
> +	    # switch back to blocking mode (else later shell commands will fail).
> +	    STDIN->blocking(1);
> +
> +	    if ($web_socket->connected) {
> +		# close connection
> +		my $msg = "\x88" . pack('N', 0) . pack('n', 0); # Opcode, mask, statuscode
> +		$web_socket->syswrite($msg);
> +		close($web_socket);
>  	    }
> +
> +	    # Reset the terminal parameters.
> +	    syswrite(\*STDOUT, "\e[24H\r\n");
> +	    PVE::PTY::tcsetattr(*STDIN, $old_termios);
>  	};
> -	print "ERROR: " . $@ . ".\n" if $@;
> +	warn $@ if $@; # show cleanup errors
>  
> -	$client_exit->($select, $web_socket, $old_termios);
> +	print STDERR "\nERROR: $err" if $err;
>  
> -	return undef
> +	return undef;
>      }});
>  
>  __PACKAGE__->register_method ({
> -- 
> 2.11.0