[pve-devel] [PATCH v2 storage 1/1] Fix #318: Delete vzdump log when deleting a backup

[Thomas Lamprecht]

missing short rationale and reasons for your approach here as
commit message, also why are you unsure about vtype backup check?

On 4/4/19 11:38 AM, Dominic Jaeger wrote:
> Signed-off-by: Dominic Jaeger <d.jaeger at proxmox.com>
> ---


please add a patch changelog here, see:
https://pve.proxmox.com/wiki/Developer_Documentation#Preparing_Patches

also, for a single patch you can send a cover-letter, but is
not really necessary, it can be easier to process 

>  PVE/API2/Storage/Content.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
> index e941cb6..5402096 100644
> --- a/PVE/API2/Storage/Content.pm
> +++ b/PVE/API2/Storage/Content.pm
> @@ -307,6 +307,12 @@ __PACKAGE__->register_method ({
>  	}
>  
>  	PVE::Storage::vdisk_free ($cfg, $volid);
> +	if ($vtype eq 'backup') {
> +	    my(undef, undef, $suffix) = File::Basename::fileparse($path,qr/(\.[^.]*)*/);

maybe better just a simple regex check which bases of our
vzdump backup name format? something like (untested):

if ($vtype eq 'backup' && $path =~ /\/(vzdump-\w+-\d+-%04d_%02d_%02d-%02d_%02d_%02d)[^\/]+$/) {
    my $log_fn = "$1.log";
    
    unlink $log_fn ...
}

> +	    $path =~ s/$suffix/.log/;
> +	    $volid =~ s/$suffix/.log/;

why do you change $volid? not used below?
Also, if the suffix is somewhere else in the path you will
replace it there too and break stuff.. hooking your regexes would help here, 
modifying existing variables to assemble something completely different can
be a bit dangerous, if later code is added below using $path expecting that it
_is_ $path your in place modifications break those assumption, but only in the
case of backups, not disks, etc, thus it may slip through testing.


> +	    unlink($path) || die "unlink '$path' failed - $!\n";
> +	}
>  
>  	return undef;
>      }});
>