[pve-devel] [PATCH storage] api: status: fix unlink on file upload

[pve-devel] [PATCH storage] api: status: fix unlink on file upload

[Lorenz Stechauner]

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];
 	}
 
 	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";
-- 
2.20.1

Re: [pve-devel] [PATCH storage] api: status: fix unlink on file upload

[Thomas Lamprecht]

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";
>