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

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

[Fabian Ebner]

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 (==)

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

 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}) {
-- 
2.20.1

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

[Stefan Reiter]

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}) {
> 

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

[Thomas Lamprecht]

On 09.11.20 09:56, 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 (==)
> 
> Does this patch work with your use case as well or is there something off?
> 
>  PVE/VZDump.pm | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
>

applied, thanks! But I really want tests for this after the releases...