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

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

[Fiona Ebner]

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

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

[Thomas Lamprecht]

Am 29/07/2024 um 16:29 schrieb Fiona Ebner:
> 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.

fine by me, but out of interest: any reason for why this is done now,
i.e., did this really cause trouble or is this done out of precaution?


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

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

[Fiona Ebner]

Am 29.07.24 um 18:50 schrieb Thomas Lamprecht:
> Am 29/07/2024 um 16:29 schrieb Fiona Ebner:
>> 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.
> 
> fine by me, but out of interest: any reason for why this is done now,
> i.e., did this really cause trouble or is this done out of precaution?

Sorry, while the commit message hints at it with "protect against
potential issues", I could've been more explicit. I stumbled upon an old
branch with this patch. Most likely I wrote it after needing the same
logic with unlink for something else. I don't know about any concrete
issues.


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

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

[Thomas Lamprecht]

Am 29/07/2024 um 16:29 schrieb Fiona Ebner:
> 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(-)
> 
>

applied, thanks!


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