From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <t.lamprecht@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; Thu, 17 Jun 2021 14:56:19 +0200 (CEST)
Message-ID: <f1addafb-13fe-f817-512f-5c2a478e7fc6@proxmox.com>
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 <pve-devel@lists.proxmox.com>,
Lorenz Stechauner <l.stechauner@proxmox.com>
References: <20210609102218.59810-1-l.stechauner@proxmox.com>
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=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 <l.stechauner@proxmox.com>
> ---
> 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";
>