Re: [pve-devel] [PATCH manager] fix maxfiles behavior

[Stefan Reiter <s.reiter@proxmox.com>]

On 11/9/20 9:56 AM, Fabian Ebner wrote:
> Commit 5ba2a605ac14de58572f7b8d6e04b45b34724b0a hard-coded 0 as the default
> for maxfiles in the --storage case, but the actual default should be the
> value from read_vzdump_defaults(), which obtains the value from
> /etc/vzdump.conf or the VZDump schema if the value has not been modified in
> that file. The initial default from the schema is 1, not 0.
> Tested on PVE 6.1 to verify that behavior.
> 
> Move the sanity check for zero-ness to where we have the final value for
> maxfiles. Like this, we also have an implicit definedness check and more
> importantly, it is more future-proof in case we ever allow maxfiles 0 in the
> VZDump schema itself.
> 
> Also, force conversion to int to be extra safe.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> @Stefan: I wasn't able to trigger a warning about using '== 0' on a non-number type,
> the only thing I can get is:
> Use of uninitialized value in numeric eq (==)

Hm, maybe I got the message mixed up.

> 
> Does this patch work with your use case as well or is there something off?

Works fine!

Sorry for the 'wrong' fix then, I only found the bug because some of my 
automated tests stopped working and didn't look to deeply into it. 
Thanks for the followup :)

> 
>   PVE/VZDump.pm | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index 517becb1..40a5a035 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -474,11 +474,7 @@ sub new {
>   
>   	    if (!defined($opts->{'prune-backups'}) && !defined($opts->{maxfiles})) {
>   		$opts->{'prune-backups'} = $info->{'prune-backups'};
> -		$opts->{maxfiles} = $info->{maxfiles} // 0;
> -		if ($opts->{maxfiles} == 0) {
> -		    # zero means keep all, so avoid triggering any remove code path to be safe
> -		    $opts->{remove} = 0;
> -		}
> +		$opts->{maxfiles} = $info->{maxfiles};
>   	    }
>   	}
>       } elsif ($opts->{dumpdir}) {
> @@ -490,7 +486,13 @@ sub new {
>   
>       if (!defined($opts->{'prune-backups'})) {
>   	my $maxfiles = delete $opts->{maxfiles} // $defaults->{maxfiles};
> -	$opts->{'prune-backups'} = { 'keep-last' => $maxfiles } if $maxfiles;
> +	$maxfiles = int($maxfiles); # shouldn't be necessary, but be safe
> +	if ($maxfiles) {
> +	    $opts->{'prune-backups'} = { 'keep-last' => $maxfiles };
> +	} else {
> +	    # maxfiles being zero means keep all, so avoid triggering any remove code path to be safe
> +	    $opts->{remove} = 0;
> +	}
>       }
>   
>       if ($opts->{tmpdir} && ! -d $opts->{tmpdir}) {
>