From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 331B69666D for ; Tue, 16 Apr 2024 10:32:38 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0A78616193 for ; Tue, 16 Apr 2024 10:32:08 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 16 Apr 2024 10:32:03 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 44CBE40309 for ; Tue, 16 Apr 2024 10:32:03 +0200 (CEST) Message-ID: <3dc7df15-716a-4c6c-a946-ddfd3d13b845@proxmox.com> Date: Tue, 16 Apr 2024 10:31:57 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Mira Limbeck References: <20240415150311.204138-1-m.limbeck@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20240415150311.204138-1-m.limbeck@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.071 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH storage] fix insecure migration failing if waiting on lock X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Apr 2024 08:32:38 -0000 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 > --- > 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;