[pve-devel] [PATCH storage] api: upload: correctly test for result of unlink

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

It's not enough to check whether $! is set. From "perldoc perlvar":

> Many system or library calls set "errno" if they fail, to
> indicate the cause of failure. They usually do not set "errno"
> to zero if they succeed and may set "errno" to a non-zero value
> on success. This means "errno", hence $!, is meaningful only
> *immediately* after a failure:

To protect against potential issues, check the return value of unlink
and only check $! if it failed.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/API2/Storage/Status.pm | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
index dc6cc69..f86e5d3 100644
--- a/src/PVE/API2/Storage/Status.pm
+++ b/src/PVE/API2/Storage/Status.pm
@@ -461,7 +461,7 @@ __PACKAGE__->register_method ({
 	# best effort to match apl_download behaviour
 	chmod 0644, $tmpfilename;
 
-	my $err_cleanup = sub { unlink $dest; die "cleanup failed: $!\n" if $! && $! != ENOENT };
+	my $err_cleanup = sub { unlink $dest or $! == ENOENT or die "cleanup failed: $!\n" };
 
 	my $cmd;
 	if ($node ne 'localhost' && $node ne PVE::INotify::nodename()) {
@@ -513,9 +513,8 @@ __PACKAGE__->register_method ({
 	    };
 	    if (my $err = $@) {
 		# unlinks only the temporary file from the http server
-		unlink $tmpfilename;
-		warn "unable to clean up temporory file '$tmpfilename' - $!\n"
-		    if $! && $! != ENOENT;
+		unlink $tmpfilename or $! == ENOENT
+		    or warn "unable to clean up temporory file '$tmpfilename' - $!\n";
 		die $err;
 	    }
 
@@ -526,8 +525,9 @@ __PACKAGE__->register_method ({
 
 	    eval { run_command($cmd, errmsg => 'import failed'); };
 
-	    unlink $tmpfilename; # the temporary file got only uploaded locally, no need to rm remote
-	    warn "unable to clean up temporary file '$tmpfilename' - $!\n" if $! && $! != ENOENT;
+	    # the temporary file got only uploaded locally, no need to rm remote
+	    unlink $tmpfilename or $! == ENOENT
+		or warn "unable to clean up temporary file '$tmpfilename' - $!\n";
 
 	    if (my $err = $@) {
 		eval { $err_cleanup->() };
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel