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 80B2073051 for ; Thu, 17 Jun 2021 14:56:50 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6E76C1BAA1 for ; Thu, 17 Jun 2021 14:56:20 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id A0A801BA93 for ; Thu, 17 Jun 2021 14:56:19 +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 7138B46483 for ; Thu, 17 Jun 2021 14:56:19 +0200 (CEST) Message-ID: Date: Thu, 17 Jun 2021 14:56:12 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:90.0) Gecko/20100101 Thunderbird/90.0 Content-Language: en-US To: Proxmox VE development discussion , Lorenz Stechauner References: <20210609102218.59810-1-l.stechauner@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20210609102218.59810-1-l.stechauner@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.922 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.17 Looks like a legit reply (A) 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] api: status: fix unlink on file upload 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: Thu, 17 Jun 2021 12:56:50 -0000 On 09.06.21 12:22, Lorenz Stechauner wrote: > after an error while copying the file to its destination the local > path of the destination was unlinked in every case, even when on the > destination was copied to via scp. > > Signed-off-by: Lorenz Stechauner > --- > PVE/API2/Storage/Status.pm | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm > index 897b4a7..bcc7722 100644 > --- a/PVE/API2/Storage/Status.pm > +++ b/PVE/API2/Storage/Status.pm > @@ -446,6 +446,7 @@ __PACKAGE__->register_method ({ > # we simply overwrite the destination file if it already exists > > my $cmd; > + my $err_cmd; > if ($node ne 'localhost' && $node ne PVE::INotify::nodename()) { > my $remip = PVE::Cluster::remote_node_ip($node); > > @@ -464,10 +465,12 @@ __PACKAGE__->register_method ({ > errmsg => "mkdir failed"); > > $cmd = ['/usr/bin/scp', @ssh_options, '-p', '--', $tmpfilename, "[$remip]:" . PVE::Tools::shell_quote($dest)]; > + $err_cmd = [@remcmd, '/bin/unlink', $dest]; > } else { > PVE::Storage::activate_storage($cfg, $param->{storage}); > File::Path::make_path($dirname); > $cmd = ['cp', '--', $tmpfilename, $dest]; > + $err_cmd = ['/bin/unlink', $dest]; depending on usr-merge[0] status unlink may not be in /bin/ but in /usr/bin/ In general, aboslute paths for system binaries, i.e., those in any s?bin dir, is not ideal rather just use the basename, and if required setup PATH (scoped) to a sensible value. That naturally does not holds true for binaries of some daemons or other tools not deemed to be for interactive execution, e.g., things in /usr/libexec or the like, but fortunately it also seldom makes sense to call those dynamically, at least if not owned by the same package (and thus having control over the path). [0]: https://wiki.debian.org/UsrMerge > } > > my $worker = sub { > @@ -481,7 +484,7 @@ __PACKAGE__->register_method ({ > > eval { PVE::Tools::run_command($cmd, errmsg => 'import failed'); }; > if (my $err = $@) { > - unlink $dest; > + eval { PVE::Tools::run_command($err_cmd); }; > die $err; > } > print "finished file import successfully\n"; >