Re: [pve-devel] [PATCH v2 storage] fix insecure migration failing if waiting on lock

[Fiona Ebner <f.ebner@proxmox.com>]

Am 16.04.24 um 13:45 schrieb Mira Limbeck:
> +	    my $handle_insecure_migration = sub {
> +		my $line = shift;
> +
> +		if (!$ip) {
> +		    ($ip) = $line =~ /^($PVE::Tools::IPRE)$/ # untaint
> +			or die "no tunnel IP received, got '$line'\n";
> +		} elsif (!$port) {
> +		    ($port) = $line =~ /^(\d+)$/ # untaint
> +			or die "no tunnel port received, got '$line'\n";
> +
> +		    # create socket, run command
> +		    $socket = IO::Socket::IP->new(PeerHost => $ip, PeerPort => $port, Type => SOCK_STREAM)

While the line too long style nits are pre-existing, it gets slightly
worse with the added indentation, so let's fix them.

Style nit: line too long (and we'll need to turn the "or die" into a
separate statement "die ... if !$socket" since we only want the "or die"
for one-liners)

> +			or die "failed to connect to tunnel at $ip:$port\n";
> +		    # we won't be reading from the socket
> +		    shutdown($socket, 0);
> +
> +		    eval { run_command($cmds, output => '>&'.fileno($socket), errfunc => $match_volid_and_log); };

Style nit: line too long

> +		    $send_error = $@;
> +
> +		    # don't close the connection entirely otherwise the receiving end
> +		    # might not get all buffered data (and fails with 'connection reset by peer')
> +		    shutdown($socket, 1);
> +		} else {
> +		    $match_volid_and_log->("[$target_sshinfo->{name}] $line");
> +		}
> +	    };
>  
> -	    # now close the socket
> -	    close($socket);
> -	    if (!close($info)) { # does waitpid()
> -		die "import failed: $!\n" if $!;
> -		die "import failed: exit code ".($?>>8)."\n";
> +	    eval { run_command($recv, outfunc => $handle_insecure_migration, errfunc => $match_volid_and_log); };

Style nit: line too long

To stay consistent with current output, we should also add the
"[$target_sshinfo->{name}]" prefix in the errfunc. Probably fits as an
in-line sub { ... } after splitting the line ;)

If we do not need the chomp below (do we?), the code between here

> +	    if (my $err = $@) {
> +		close($socket) if $socket;
> +
> +		chomp($err);
> +		die "failed to run insecure migration: $err\n";
>  	    }
>  
> +	    # now close the socket
> +	    close($socket) if $socket;

and here could also just become

my $err = $@;
close($socket) if $socket;
die "failed to run insecure migration: $err" if $err;

Or maybe even call it $recv_error for consistency with $send_error.

>  	    die $send_error if $send_error;
>  	} else {
>  	    push @$cmds, $recv;