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

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

Am 15.04.24 um 17:03 schrieb Mira Limbeck:
> both STDOUT and STDERR are written into `$info` which is then parsed for
> IP and port of the target socket listening.
> when the ports file can't be locked immediately `trying to acquire
> lock...` is printed on STDERR and in turn written into `$info`.
> trying to parse the IP then fails, resulting in a migration or
> replication failing.
> 
> the bare open3 call is replaced by the run_command wrapper from
> pve-common to use a safe wrapper around open3 with the same
> functionality.
> STDERR is now read separately from STDOUT and the last line of STDERR
> is kept in case of errors.
> 

Please log all of STDERR and even if there are no errors. Like that, we
won't miss anything interesting. For example, the message

> volume lvmthin/vm-110-disk-1 already exists - importing with a
different name

which is printed via warn on the remote side will not be logged after
your patch anymore.

Fixes: 57acd6a ("fix #1452: also log stderr of remote command with
insecure storage migration")

> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
> I've replaced open3 with run_command on recommendation from others. tt
> complicates the logic a little bit compared to the `simple` open3
> solution, but may be less error prone since run_command abstracts open3
> nicely behind a safe wrapper.
> 
> one thing I could not verify was the now removed line:
> `if (!close($info)) { # does waitpid()`
> 
> I could not find any information mentioning this behavior of close() on
> a simple file.
> 

I guess this comment was outdated after 57acd6a ("fix #1452: also log
stderr of remote command with insecure storage migration"). Before that
it was a pipe.

> 
>  src/PVE/Storage.pm | 79 +++++++++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 36 deletions(-)
> 
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 40314a8..b6045d5 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -851,44 +851,51 @@ sub storage_migrate {
>  
>      eval {
>  	if ($insecure) {

(...)

> +	    my $last_err = '';
> +	    my $ip;
> +	    my $port;
> +	    my $socket;
> +	    my $send_error;
> +	    my $handle_insecure_migration = sub {

Style nit: maybe group the two error variables and have a blank between
the variables and the closure.

> +		my $line = shift;
> +
> +		if (!$ip) {
> +		    if ($line =~ /^($PVE::Tools::IPRE)$/) {
> +			$ip = $1;
> +		    } else {
> +			die "no tunnel IP received, got $line\n";
> +		    }

Would be nice to not drop the untaint comment

Style nit: could've kept the previous way to write this that fits in two
lines

> +		} elsif(!$port) {

Style nit: missing space

> +		    if ($line =~ /^(\d+)$/) {
> +			$port = $1;
> +		    } else {
> +			die "no tunnel port received, got $line\n";
> +		    }
> +		}
> +		if ($ip && $port && !$socket) {
> +		    # create socket, run command
> +		    $socket = IO::Socket::IP->new(PeerHost => $ip, PeerPort => $port, Type => SOCK_STREAM)
> +			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); };
> +		    $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);
> +		} elsif ($ip && $port) {
> +		    $match_volid_and_log->("[$target_sshinfo->{name}] $line");
> +		}
> +	    };

Style nit: a blank would be nice here for better separation

The closure could also be structured with explicit returns like:

if (!$ip)
  $ip = ...
  return

if (!$port)
  $port = ...
  create socket, run command
  return

match_volid_and_log

Seems a bit easier to read IMHO, because it saves upon a few
conditions/else branches. What do you think?

> +	    eval { run_command($recv, outfunc => $handle_insecure_migration, errfunc => sub { $last_err = shift; }); };
> +	    if (my $err = $@) {

Shouldn't we try to close the socket here too before dying?

> +		chomp($err);
> +		die "failed to run insecure migration: $err - $last_err\n";
>  	    }
> -

Style nit: I'd prefer to keep this blank for better separation

>  	    # now close the socket
> -	    close($socket);
> -	    if (!close($info)) { # does waitpid()
> -		die "import failed: $!\n" if $!;
> -		die "import failed: exit code ".($?>>8)."\n";
> -	    }
> -
> +	    close($socket) if $socket;
>  	    die $send_error if $send_error;
>  	} else {
>  	    push @$cmds, $recv;