[pve-devel] [PATCH guest-common 1/4] vzdump: loosen mailto pattern to allow whitespaces

[pve-devel] [PATCH guest-common 1/4] vzdump: loosen mailto pattern to allow whitespaces

[Fabian Ebner]

to make it more compatible to what we had previously. In pve-manger,
split_list() is called on the parameter, so any whitespaces and even stand-alone
',' and ';' are taken care of, leaving only the real addresses. This also fixes
creating a backup job with empty mailto in the UI.

For example,
> vzdump 153 --mailto " ,,,developer@proxmox.com; admin@proxmox.com ; "
was valid and worked in PVE 6.2.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/VZDump/Common.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/VZDump/Common.pm b/PVE/VZDump/Common.pm
index af141de..fb11b3d 100644
--- a/PVE/VZDump/Common.pm
+++ b/PVE/VZDump/Common.pm
@@ -72,7 +72,7 @@ sub parse_dow {
 };
 
 my $mailto_pattern = '[a-zA-Z0-9+._@][-a-zA-Z0-9+._@]*';
-my $mailto_list_pattern = "($mailto_pattern)([;,]$mailto_pattern)*";
+my $mailto_list_pattern = qr/([;,]?(?:$mailto_pattern|\s*))*/;
 
 my $confdesc = {
     vmid => {
-- 
2.20.1

[pve-devel] [PATCH manager 2/4] vzdump: refactor parsing mailto and exclude-path

[Fabian Ebner]

so it can be mocked.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/VZDump.pm | 11 +----------
 PVE/VZDump.pm      | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 806ac7fd..44376106 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -88,16 +88,7 @@ __PACKAGE__->register_method ({
 	# silent exit if specified VMs run on other nodes
 	return "OK" if !scalar(@{$local_vmids}) && !$param->{all};
 
-	# exclude-path list need to be 0 separated
-	if (defined($param->{'exclude-path'})) {
-	    my @expaths = split(/\0/, $param->{'exclude-path'} || '');
-	    $param->{'exclude-path'} = [ @expaths ];
-	}
-
-	if (defined($param->{mailto})) {
-	    my @mailto = PVE::Tools::split_list(extract_param($param, 'mailto'));
-	    $param->{mailto} = [ @mailto ];
-	}
+	PVE::VZDump::parse_mailto_exclude_path($param);
 
 	die "you can only backup a single VM with option --stdout\n"
 	    if $param->{stdout} && scalar(@{$local_vmids}) != 1;
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index a99d0565..2ddfa851 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -1177,6 +1177,27 @@ sub option_exists {
     return defined($confdesc->{$key});
 }
 
+# NOTE it might make sense to merge this and verify_vzdump_parameters(), but one
+# needs to adapt command_line() in guest-common's PVE/VZDump/Common.pm and detect
+# a second parsing attempt, because verify_vzdump_parameters() is called twice
+# during the update_job API call.
+sub parse_mailto_exclude_path {
+    my ($param) = @_;
+
+    # exclude-path list need to be 0 separated
+    if (defined($param->{'exclude-path'})) {
+	my @expaths = split(/\0/, $param->{'exclude-path'} || '');
+	$param->{'exclude-path'} = [ @expaths ];
+    }
+
+    if (defined($param->{mailto})) {
+	my @mailto = PVE::Tools::split_list(extract_param($param, 'mailto'));
+	$param->{mailto} = [ @mailto ];
+    }
+
+    return;
+}
+
 sub verify_vzdump_parameters {
     my ($param, $check_missing) = @_;
 
-- 
2.20.1

[pve-devel] [PATCH manager 3/4] test: vzdump: add tests for mailto

[Fabian Ebner]

Re-use the existing code, by allowing special kinds of "tests" that just set
the options that are tested for.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Patch #1 and thus a dependency bump on guest-common are needed

 test/vzdump_new_retention_test.pl | 174 +++++++++++++++++++++++++++++-
 1 file changed, 172 insertions(+), 2 deletions(-)

diff --git a/test/vzdump_new_retention_test.pl b/test/vzdump_new_retention_test.pl
index 569419fb..a8f70d62 100755
--- a/test/vzdump_new_retention_test.pl
+++ b/test/vzdump_new_retention_test.pl
@@ -73,7 +73,7 @@ $pve_tools_module->mock(
     },
 );
 
-my @tested_options = qw(prune-backups remove);
+my $tested_options;
 
 # each test consists of the following:
 # name          - unique name for the test
@@ -81,7 +81,13 @@ my @tested_options = qw(prune-backups remove);
 # storage_param - parameters for the mocked storage configuration
 # vzdump_param  - parameters for the mocked /etc/vzdump.conf
 # expected      - expected options
+#
+# To begin testing for different options, use a fake test like the first one
 my @tests = (
+    {
+	description => 'BEGIN RETENTION TESTS',
+	tested_options => ['prune-backups', 'remove'],
+    },
     {
 	description => 'no params',
 	expected => {
@@ -464,11 +470,174 @@ my @tests = (
 	    remove => 0,
 	},
     },
+    {
+	description => 'BEGIN MAILTO TESTS',
+	tested_options => ['mailto'],
+    },
+    {
+	description => 'mailto vzdump 1',
+	vzdump_param => {
+	    'mailto' => 'developer@proxmox.com',
+	},
+	expected => {
+	    'mailto' => [
+		'developer@proxmox.com',
+	    ],
+	},
+    },
+    {
+	description => 'mailto vzdump 2',
+	vzdump_param => {
+	    'mailto' => 'developer@proxmox.com admin@proxmox.com',
+	},
+	expected => {
+	    'mailto' => [
+		'developer@proxmox.com',
+		'admin@proxmox.com',
+	    ],
+	},
+    },
+    {
+	description => 'mailto vzdump 3',
+	vzdump_param => {
+	    'mailto' => 'developer@proxmox.com,admin@proxmox.com',
+	},
+	expected => {
+	    'mailto' => [
+		'developer@proxmox.com',
+		'admin@proxmox.com',
+	    ],
+	},
+    },
+    {
+	description => 'mailto vzdump 4',
+	vzdump_param => {
+	    'mailto' => 'developer@proxmox.com, admin@proxmox.com',
+	},
+	expected => {
+	    'mailto' => [
+		'developer@proxmox.com',
+		'admin@proxmox.com',
+	    ],
+	},
+    },
+    {
+	description => 'mailto vzdump 5',
+	vzdump_param => {
+	    'mailto' => ' ,,; developer@proxmox.com, ; admin@proxmox.com ',
+	},
+	expected => {
+	    'mailto' => [
+		'developer@proxmox.com',
+		'admin@proxmox.com',
+	    ],
+	},
+    },
+    {
+	description => 'mailto vzdump 6',
+	vzdump_param => {
+	    'mailto' => '',
+	},
+	expected => {
+	    'mailto' => [],
+	},
+    },
+    {
+	description => 'mailto CLI 1',
+	cli_param => {
+	    'mailto' => 'developer@proxmox.com',
+	},
+	expected => {
+	    'mailto' => [
+		'developer@proxmox.com',
+	    ],
+	},
+    },
+    {
+	description => 'mailto CLI 2',
+	cli_param => {
+	    'mailto' => 'developer@proxmox.com admin@proxmox.com',
+	},
+	expected => {
+	    'mailto' => [
+		'developer@proxmox.com',
+		'admin@proxmox.com',
+	    ],
+	},
+    },
+    {
+	description => 'mailto CLI 3',
+	cli_param => {
+	    'mailto' => 'developer@proxmox.com,admin@proxmox.com',
+	},
+	expected => {
+	    'mailto' => [
+		'developer@proxmox.com',
+		'admin@proxmox.com',
+	    ],
+	},
+    },
+    {
+	description => 'mailto CLI 4',
+	cli_param => {
+	    'mailto' => 'developer@proxmox.com, admin@proxmox.com',
+	},
+	expected => {
+	    'mailto' => [
+		'developer@proxmox.com',
+		'admin@proxmox.com',
+	    ],
+	},
+    },
+    {
+	description => 'mailto CLI 5',
+	cli_param => {
+	    'mailto' => ' ,,; developer@proxmox.com, ; admin@proxmox.com ',
+	},
+	expected => {
+	    'mailto' => [
+		'developer@proxmox.com',
+		'admin@proxmox.com',
+	    ],
+	},
+    },
+    {
+	description => 'mailto both 1',
+	vzdump_param => {
+	    'mailto' => 'developer@proxmox.com',
+	},
+	cli_param => {
+	    'mailto' => 'admin@proxmox.com',
+	},
+	expected => {
+	    'mailto' => [
+		'admin@proxmox.com',
+	    ],
+	},
+    },
+    {
+	description => 'mailto both 2',
+	vzdump_param => {
+	    'mailto' => 'developer@proxmox.com',
+	},
+	cli_param => {
+	    'mailto' => '',
+	},
+	expected => {
+	    'mailto' => [],
+	},
+    },
 );
 
 plan tests => scalar @tests;
 
 foreach my $test (@tests) {
+    if (defined($test->{tested_options})) {
+	$tested_options = $test->{tested_options};
+	ok(1, $test->{description});
+	next;
+    }
+
     prepare_storage_config($test->{storage_param});
     prepare_vzdump_config($test->{vzdump_param});
 
@@ -477,6 +646,7 @@ foreach my $test (@tests) {
 
     my $got = eval {
 	PVE::VZDump::verify_vzdump_parameters($test->{cli_param}, 1);
+	PVE::VZDump::parse_mailto_exclude_path($test->{cli_param});
 
 	my $vzdump = PVE::VZDump->new('fake cmdline', $test->{cli_param}, undef);
 
@@ -484,7 +654,7 @@ foreach my $test (@tests) {
 	die "maxfiles is defined" if defined($opts->{maxfiles});
 
 	my $res = {};
-	foreach my $opt (@tested_options) {
+	foreach my $opt (@{$tested_options}) {
 	    next if !defined($opts->{$opt});
 	    $res->{$opt} = $opts->{$opt};
 	}
-- 
2.20.1

[pve-devel] [PATCH manager 4/4] test: vzdump: rename vzdump_new_retention_test.pl to vzdump_new_test.pl

[Fabian Ebner]

as it now also includes tests for new() with non-retention options.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 test/Makefile                                             | 8 ++++----
 test/{vzdump_new_retention_test.pl => vzdump_new_test.pl} | 0
 2 files changed, 4 insertions(+), 4 deletions(-)
 rename test/{vzdump_new_retention_test.pl => vzdump_new_test.pl} (100%)

diff --git a/test/Makefile b/test/Makefile
index d383e89f..eb6f38f1 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -18,14 +18,14 @@ replication%.t: replication_test%.pl
 test-mail:
 	./mail_test.pl
 
-test-vzdump: test-vzdump-guest-included test-vzdump-retention
+test-vzdump: test-vzdump-guest-included test-vzdump-new
 
-.PHONY: test-vzdump-guest-included test-vzdump-retention
+.PHONY: test-vzdump-guest-included test-vzdump-new
 test-vzdump-guest-included:
 	./vzdump_guest_included_test.pl
 
-test-vzdump-retention:
-	./vzdump_new_retention_test.pl
+test-vzdump-new:
+	./vzdump_new_test.pl
 
 .PHONY: install
 install:
diff --git a/test/vzdump_new_retention_test.pl b/test/vzdump_new_test.pl
similarity index 100%
rename from test/vzdump_new_retention_test.pl
rename to test/vzdump_new_test.pl
-- 
2.20.1

Re: [pve-devel] [PATCH guest-common 1/4] vzdump: loosen mailto pattern to allow whitespaces

[Fabian Ebner]

I'm going to try and come up with a v2 that re-uses the more complete 
'email' pattern from pve-common.

On 12.02.21 13:23, Fabian Ebner wrote:
> to make it more compatible to what we had previously. In pve-manger,
> split_list() is called on the parameter, so any whitespaces and even stand-alone
> ',' and ';' are taken care of, leaving only the real addresses. This also fixes
> creating a backup job with empty mailto in the UI.
> 
> For example,
>> vzdump 153 --mailto " ,,,developer@proxmox.com; admin@proxmox.com ; "
> was valid and worked in PVE 6.2.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>   PVE/VZDump/Common.pm | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/PVE/VZDump/Common.pm b/PVE/VZDump/Common.pm
> index af141de..fb11b3d 100644
> --- a/PVE/VZDump/Common.pm
> +++ b/PVE/VZDump/Common.pm
> @@ -72,7 +72,7 @@ sub parse_dow {
>   };
>   
>   my $mailto_pattern = '[a-zA-Z0-9+._@][-a-zA-Z0-9+._@]*';
> -my $mailto_list_pattern = "($mailto_pattern)([;,]$mailto_pattern)*";
> +my $mailto_list_pattern = qr/([;,]?(?:$mailto_pattern|\s*))*/;
>   
>   my $confdesc = {
>       vmid => {
>