Re: [pve-devel] [PATCH storage] status: add unlink to worker in file upload

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 21.07.21 14:52, Lorenz Stechauner wrote:
> this is the first step in which not the http server removes the
> temporary file, but the worker itself. for now, this `unlink`
> statement won't unlink anything existing. but after changes to
> the http server have been made, the endpoint will be able to
> clean up for itself afterwards.
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
> a version bump would be helpful, so the older versions can be
> marked as 'breaks' in the pve-http-server repo after the above
> mentioned changes.
> 

You do not necesarrily need to send it out as its own patch, IMO this
would be still benefit from being grouped in a series with the other changes
that are related to the overall thing you want to achieve.

As mentioned in the review, it's totally fine if you do not actually do
the breaks, but you should be aware of the need for it and also mention
it to ensure a maintainer does not forgets on actually applying + bumping.

>  PVE/API2/Storage/Status.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index 72fd851..9275d1f 100644
> --- a/PVE/API2/Storage/Status.pm
> +++ b/PVE/API2/Storage/Status.pm
> @@ -478,6 +478,7 @@ __PACKAGE__->register_method ({
>  	    print "command: " . join(' ', @$cmd) . "\n";
>  
>  	    eval { PVE::Tools::run_command($cmd, errmsg => 'import failed'); };
> +	    unlink $tmpfilename;

error handling still missing for the unlink. As unlink returns the files actually
unlinked (it accepts a list) it's easy to do for the single-file case:

unlink $tmpfilename or warn "bla - $!\n";


>  	    if (my $err = $@) {
>  		unlink $dest;
>  		die $err;
>