[pve-devel] [PATCH-SERIES v2] loosen up mailto regex for backwards compatibility

[pve-devel] [PATCH-SERIES v2] loosen up mailto regex for backwards compatibility

[Fabian Ebner]

especially regarding the whitespace-agnostic behavior. And while we're at it,
also use the more complete email regex from pve-common.

Changes from v1:
    * re-use the email regex from pve-common
    * improve printing out mailto parameters to the cron file


common:

Fabian Ebner (2):
  sendmail: use more complete email regex and shellquote
  register email-or-username format

 src/PVE/JSONSchema.pm | 14 +++++++++++++-
 src/PVE/Tools.pm      | 17 ++++++++++++-----
 2 files changed, 25 insertions(+), 6 deletions(-)


guest-common:

Fabian Ebner (3):
  vzdump: command line: refactor handling prune-backups
  vzdump: command line: make sure mailto is comma-separated
  vzdump: mailto: use email-or-username-list format

 PVE/VZDump/Common.pm | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)


manager:

Fabian Ebner (3):
  vzdump: refactor parsing mailto so it can be mocked
  test: vzdump: add tests for mailto
  test: vzdump: rename vzdump_new_retention_test.pl to
    vzdump_new_test.pl

 PVE/API2/VZDump.pm                            |  11 +-
 PVE/VZDump.pm                                 |  21 +++
 test/Makefile                                 |   8 +-
 ...w_retention_test.pl => vzdump_new_test.pl} | 174 +++++++++++++++++-
 4 files changed, 198 insertions(+), 16 deletions(-)
 rename test/{vzdump_new_retention_test.pl => vzdump_new_test.pl} (74%)

-- 
2.20.1

[pve-devel] [PATCH v2 common 1/8] sendmail: use more complete email regex and shellquote

[Fabian Ebner]

Shellquote is needed for '~', and while it doesn't help with '-', there should
be no problem, because options are separated from mailto since commit
216a3f4f131693dc4bbad5e06e96a61baef5f5e9.

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

New in v2.

Since JSONSchema already uses Tools, the pattern has to live in Tools.

 src/PVE/JSONSchema.pm |  2 +-
 src/PVE/Tools.pm      | 17 ++++++++++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index 29ada5b..5870b69 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -471,7 +471,7 @@ register_format('email', \&pve_verify_email);
 sub pve_verify_email {
     my ($email, $noerr) = @_;
 
-    if ($email !~ /^[\w\+\-\~]+(\.[\w\+\-\~]+)*@[a-zA-Z0-9\-]+(\.[a-zA-Z0-9\-]+)*$/) {
+    if ($email !~ /^$PVE::Tools::EMAIL_RE$/) {
 	   return undef if $noerr;
 	   die "value does not look like a valid email address\n";
     }
diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 7fefa52..fc4a367 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -87,6 +87,9 @@ our $IPV6RE = "(?:" .
 
 our $IPRE = "(?:$IPV4RE|$IPV6RE)";
 
+our $EMAIL_USER_RE = qr/[\w\+\-\~]+(\.[\w\+\-\~]+)*/;
+our $EMAIL_RE = qr/$EMAIL_USER_RE@[a-zA-Z0-9\-]+(\.[a-zA-Z0-9\-]+)*/;
+
 use constant {CLONE_NEWNS   => 0x00020000,
               CLONE_NEWUTS  => 0x04000000,
               CLONE_NEWIPC  => 0x08000000,
@@ -1469,23 +1472,27 @@ sub sync_mountpoint {
 # mailto may be a single email string or an array of receivers
 sub sendmail {
     my ($mailto, $subject, $text, $html, $mailfrom, $author) = @_;
-    my $mail_re = qr/[^-a-zA-Z0-9+._@]/;
 
     $mailto = [ $mailto ] if !ref($mailto);
 
+    my $mailto_quoted = [];
     for my $to (@$mailto) {
-	die "illegal character in mailto address\n" if $to =~ $mail_re;
+	die "mailto does not look like a valid email address or username\n"
+	    if $to !~ /^$EMAIL_RE$/ && $to !~ /^$EMAIL_USER_RE$/;
+	push @$mailto_quoted, shellquote($to);
     }
 
     my $rcvrtxt = join (', ', @$mailto);
 
     $mailfrom = $mailfrom || "root";
-    die "illegal character in mailfrom address\n" if $mailfrom =~ $mail_re;
+    die "mailfrom does not look like a valid email address or username\n"
+	    if $mailfrom !~ /^$EMAIL_RE$/ && $mailfrom !~ /^$EMAIL_USER_RE$/;
+    my $mailfrom_quoted = shellquote($mailfrom);
 
     $author = $author // 'Proxmox VE';
 
-    open (MAIL, "|-", "sendmail", "-B", "8BITMIME", "-f", $mailfrom, "--", @$mailto) ||
-	die "unable to open 'sendmail' - $!";
+    open (MAIL, "|-", "sendmail", "-B", "8BITMIME", "-f", $mailfrom_quoted,
+	"--", @$mailto_quoted) || die "unable to open 'sendmail' - $!";
 
     my $date = time2str('%a, %d %b %Y %H:%M:%S %z', time());
 
-- 
2.20.1

[pve-devel] applied: [PATCH v2 common 1/8] sendmail: use more complete email regex and shellquote

[Thomas Lamprecht]

On 15.02.21 13:24, Fabian Ebner wrote:
> Shellquote is needed for '~', and while it doesn't help with '-', there should
> be no problem, because options are separated from mailto since commit
> 216a3f4f131693dc4bbad5e06e96a61baef5f5e9.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> New in v2.
> 
> Since JSONSchema already uses Tools, the pattern has to live in Tools.
> 
>  src/PVE/JSONSchema.pm |  2 +-
>  src/PVE/Tools.pm      | 17 ++++++++++++-----
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
>

applied, thanks!

[pve-devel] [PATCH v2 common 2/8] register email-or-username format

[Fabian Ebner]

To be used for the mailto vzdump parameter.

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

New in v2.

 src/PVE/JSONSchema.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index 5870b69..20d72b3 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -478,6 +478,18 @@ sub pve_verify_email {
     return $email;
 }
 
+register_format('email-or-username', \&pve_verify_email_or_username);
+sub pve_verify_email_or_username {
+    my ($email, $noerr) = @_;
+
+    if ($email !~ /^$PVE::Tools::EMAIL_RE$/ &&
+	$email !~ /^$PVE::Tools::EMAIL_USER_RE$/) {
+	   return undef if $noerr;
+	   die "value does not look like a valid email address or user name\n";
+    }
+    return $email;
+}
+
 register_format('dns-name', \&pve_verify_dns_name);
 sub pve_verify_dns_name {
     my ($name, $noerr) = @_;
-- 
2.20.1

[pve-devel] applied: [PATCH v2 common 2/8] register email-or-username format

[Thomas Lamprecht]

On 15.02.21 13:24, Fabian Ebner wrote:
> To be used for the mailto vzdump parameter.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> New in v2.
> 
>  src/PVE/JSONSchema.pm | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
>

applied, thanks!

[pve-devel] [PATCH v2 guest-common 3/8] vzdump: command line: refactor handling prune-backups

[Fabian Ebner]

to re-use a line here and with the next patch.

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

New in v2.

 PVE/VZDump/Common.pm | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/PVE/VZDump/Common.pm b/PVE/VZDump/Common.pm
index af141de..eceea7f 100644
--- a/PVE/VZDump/Common.pm
+++ b/PVE/VZDump/Common.pm
@@ -393,11 +393,10 @@ sub command_line {
 	    foreach my $path (split(/\0/, $v || '')) {
 		$cmd .= " --$p " . PVE::Tools::shellquote($path);
 	    }
-	} elsif ($p eq 'prune-backups') {
-	    my $property_string = PVE::JSONSchema::print_property_string($v, 'prune-backups');
-	    $cmd .= " --$p " . PVE::Tools::shellquote($property_string)
-		if defined($property_string) && $property_string ne '';
 	} else {
+	    $v = PVE::JSONSchema::print_property_string($v, 'prune-backups')
+		if $p eq 'prune-backups';
+
 	    $cmd .= " --$p " . PVE::Tools::shellquote($v) if defined($v) && $v ne '';
 	}
     }
-- 
2.20.1

[pve-devel] [PATCH v2 guest-common 4/8] vzdump: command line: make sure mailto is comma-separated

[Fabian Ebner]

In addition to relying on shellquote(), it's still nice to avoid printing out
unnecessary whitespaces, especially newlines.

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

New in v2.

 PVE/VZDump/Common.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PVE/VZDump/Common.pm b/PVE/VZDump/Common.pm
index eceea7f..5d93b51 100644
--- a/PVE/VZDump/Common.pm
+++ b/PVE/VZDump/Common.pm
@@ -394,6 +394,7 @@ sub command_line {
 		$cmd .= " --$p " . PVE::Tools::shellquote($path);
 	    }
 	} else {
+	    $v = join(",", PVE::Tools::split_list($v)) if $p eq 'mailto';
 	    $v = PVE::JSONSchema::print_property_string($v, 'prune-backups')
 		if $p eq 'prune-backups';
 
-- 
2.20.1

[pve-devel] [PATCH v2 guest-common 5/8] vzdump: mailto: use email-or-username-list format

[Fabian Ebner]

because it is a more complete pattern. Also, 'mailto' was a '-list' format in
PVE 6.2 and earlier, so this also fixes whitespace-related backwards
compatibility. In particular, this fixes creating a backup job in the GUI
without setting an address, which passes along ''.

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

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

Changes from v1:
    * re-use the more complete pattern from pve-common

Dependency bump for pve-common is needed.

 PVE/VZDump/Common.pm | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/PVE/VZDump/Common.pm b/PVE/VZDump/Common.pm
index 5d93b51..86cb7bd 100644
--- a/PVE/VZDump/Common.pm
+++ b/PVE/VZDump/Common.pm
@@ -71,9 +71,6 @@ sub parse_dow {
     return $res;
 };
 
-my $mailto_pattern = '[a-zA-Z0-9+._@][-a-zA-Z0-9+._@]*';
-my $mailto_list_pattern = "($mailto_pattern)([;,]$mailto_pattern)*";
-
 my $confdesc = {
     vmid => {
 	type => 'string', format => 'pve-vmid-list',
@@ -146,8 +143,7 @@ my $confdesc = {
     },
     mailto => {
 	type => 'string',
-	pattern => $mailto_list_pattern,
-	format_description => 'email-or-username-list',
+	format => 'email-or-username-list',
 	description => "Comma-separated list of email addresses or users that should" .
 	    " receive email notifications.",
 	optional => 1,
-- 
2.20.1

[pve-devel] [PATCH v2 manager 6/8] vzdump: refactor parsing mailto so it can be mocked

[Fabian Ebner]

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

No changes from v1.

 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 v2 manager 7/8] 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>
---

No changes from v1.

Dependency bump for pve-guest-common is 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 v2 manager 8/8] 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>
---

No changes from v1.

 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-SERIES v2] loosen up mailto regex for backwards compatibility

[Dominik Csapak]

LGTM, nothing obvious that is wrong
one small thing though, after this patch i cannot
have an @ in the local part of an email anymore, though
i do not think that people actually use that?

e.g. foo@bar@example.com does not work anymore

Other than that, i tested it and worked as advertised

Reviewed-By: Dominik Csapak <d.csapak@proxmox.com>

On 2/15/21 13:24, Fabian Ebner wrote:
> especially regarding the whitespace-agnostic behavior. And while we're at it,
> also use the more complete email regex from pve-common.
> 
> Changes from v1:
>      * re-use the email regex from pve-common
>      * improve printing out mailto parameters to the cron file
> 
> 
> common:
> 
> Fabian Ebner (2):
>    sendmail: use more complete email regex and shellquote
>    register email-or-username format
> 
>   src/PVE/JSONSchema.pm | 14 +++++++++++++-
>   src/PVE/Tools.pm      | 17 ++++++++++++-----
>   2 files changed, 25 insertions(+), 6 deletions(-)
> 
> 
> guest-common:
> 
> Fabian Ebner (3):
>    vzdump: command line: refactor handling prune-backups
>    vzdump: command line: make sure mailto is comma-separated
>    vzdump: mailto: use email-or-username-list format
> 
>   PVE/VZDump/Common.pm | 14 +++++---------
>   1 file changed, 5 insertions(+), 9 deletions(-)
> 
> 
> manager:
> 
> Fabian Ebner (3):
>    vzdump: refactor parsing mailto so it can be mocked
>    test: vzdump: add tests for mailto
>    test: vzdump: rename vzdump_new_retention_test.pl to
>      vzdump_new_test.pl
> 
>   PVE/API2/VZDump.pm                            |  11 +-
>   PVE/VZDump.pm                                 |  21 +++
>   test/Makefile                                 |   8 +-
>   ...w_retention_test.pl => vzdump_new_test.pl} | 174 +++++++++++++++++-
>   4 files changed, 198 insertions(+), 16 deletions(-)
>   rename test/{vzdump_new_retention_test.pl => vzdump_new_test.pl} (74%)
> 

Re: [pve-devel] [PATCH-SERIES v2] loosen up mailto regex for backwards compatibility

[Fabian Ebner]

On 17.02.21 10:52, Dominik Csapak wrote:
> LGTM, nothing obvious that is wrong
> one small thing though, after this patch i cannot
> have an @ in the local part of an email anymore, though
> i do not think that people actually use that?
> 
> e.g. foo@bar@example.com does not work anymore
> 

Thanks for the review and for catching that! I'll send a follow-up (or 
does this warrant a v3?) including '@' in the local part, and check that 
other users of the 'email' format are not confused by that in some way.

> Other than that, i tested it and worked as advertised
> 
> Reviewed-By: Dominik Csapak <d.csapak@proxmox.com>
> 
> On 2/15/21 13:24, Fabian Ebner wrote:
>> especially regarding the whitespace-agnostic behavior. And while we're 
>> at it,
>> also use the more complete email regex from pve-common.
>>
>> Changes from v1:
>>      * re-use the email regex from pve-common
>>      * improve printing out mailto parameters to the cron file
>>
>>
>> common:
>>
>> Fabian Ebner (2):
>>    sendmail: use more complete email regex and shellquote
>>    register email-or-username format
>>
>>   src/PVE/JSONSchema.pm | 14 +++++++++++++-
>>   src/PVE/Tools.pm      | 17 ++++++++++++-----
>>   2 files changed, 25 insertions(+), 6 deletions(-)
>>
>>
>> guest-common:
>>
>> Fabian Ebner (3):
>>    vzdump: command line: refactor handling prune-backups
>>    vzdump: command line: make sure mailto is comma-separated
>>    vzdump: mailto: use email-or-username-list format
>>
>>   PVE/VZDump/Common.pm | 14 +++++---------
>>   1 file changed, 5 insertions(+), 9 deletions(-)
>>
>>
>> manager:
>>
>> Fabian Ebner (3):
>>    vzdump: refactor parsing mailto so it can be mocked
>>    test: vzdump: add tests for mailto
>>    test: vzdump: rename vzdump_new_retention_test.pl to
>>      vzdump_new_test.pl
>>
>>   PVE/API2/VZDump.pm                            |  11 +-
>>   PVE/VZDump.pm                                 |  21 +++
>>   test/Makefile                                 |   8 +-
>>   ...w_retention_test.pl => vzdump_new_test.pl} | 174 +++++++++++++++++-
>>   4 files changed, 198 insertions(+), 16 deletions(-)
>>   rename test/{vzdump_new_retention_test.pl => vzdump_new_test.pl} (74%)
>>
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH-SERIES v2] loosen up mailto regex for backwards compatibility

[Fabian Ebner]

On 17.02.21 11:35, Fabian Ebner wrote:
> On 17.02.21 10:52, Dominik Csapak wrote:
>> LGTM, nothing obvious that is wrong
>> one small thing though, after this patch i cannot
>> have an @ in the local part of an email anymore, though
>> i do not think that people actually use that?
>>
>> e.g. foo@bar@example.com does not work anymore
>>
> 
> Thanks for the review and for catching that! I'll send a follow-up (or 
> does this warrant a v3?) including '@' in the local part, and check that 
> other users of the 'email' format are not confused by that in some way.
> 

After a bit of discussion, I decided to not send that follow-up, because 
'@' is technically only valid in quoted strings (which we didn't/don't 
support here) and AFAICT we used the same 'email' format from pve-common 
in some parts of PMG without noticeable complaints.

>> Other than that, i tested it and worked as advertised
>>
>> Reviewed-By: Dominik Csapak <d.csapak@proxmox.com>
>>
>> On 2/15/21 13:24, Fabian Ebner wrote:
>>> especially regarding the whitespace-agnostic behavior. And while 
>>> we're at it,
>>> also use the more complete email regex from pve-common.
>>>
>>> Changes from v1:
>>>      * re-use the email regex from pve-common
>>>      * improve printing out mailto parameters to the cron file
>>>
>>>
>>> common:
>>>
>>> Fabian Ebner (2):
>>>    sendmail: use more complete email regex and shellquote
>>>    register email-or-username format
>>>
>>>   src/PVE/JSONSchema.pm | 14 +++++++++++++-
>>>   src/PVE/Tools.pm      | 17 ++++++++++++-----
>>>   2 files changed, 25 insertions(+), 6 deletions(-)
>>>
>>>
>>> guest-common:
>>>
>>> Fabian Ebner (3):
>>>    vzdump: command line: refactor handling prune-backups
>>>    vzdump: command line: make sure mailto is comma-separated
>>>    vzdump: mailto: use email-or-username-list format
>>>
>>>   PVE/VZDump/Common.pm | 14 +++++---------
>>>   1 file changed, 5 insertions(+), 9 deletions(-)
>>>
>>>
>>> manager:
>>>
>>> Fabian Ebner (3):
>>>    vzdump: refactor parsing mailto so it can be mocked
>>>    test: vzdump: add tests for mailto
>>>    test: vzdump: rename vzdump_new_retention_test.pl to
>>>      vzdump_new_test.pl
>>>
>>>   PVE/API2/VZDump.pm                            |  11 +-
>>>   PVE/VZDump.pm                                 |  21 +++
>>>   test/Makefile                                 |   8 +-
>>>   ...w_retention_test.pl => vzdump_new_test.pl} | 174 +++++++++++++++++-
>>>   4 files changed, 198 insertions(+), 16 deletions(-)
>>>   rename test/{vzdump_new_retention_test.pl => vzdump_new_test.pl} (74%)
>>>
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied: [PATCH-SERIES v2] loosen up mailto regex for backwards compatibility

[Thomas Lamprecht]

On 15.02.21 13:24, Fabian Ebner wrote:
> especially regarding the whitespace-agnostic behavior. And while we're at it,
> also use the more complete email regex from pve-common.
> 
> Changes from v1:
>     * re-use the email regex from pve-common
>     * improve printing out mailto parameters to the cron file
> 
> 
> common:
> 
> Fabian Ebner (2):
>   sendmail: use more complete email regex and shellquote
>   register email-or-username format
> 
>  src/PVE/JSONSchema.pm | 14 +++++++++++++-
>  src/PVE/Tools.pm      | 17 ++++++++++++-----
>  2 files changed, 25 insertions(+), 6 deletions(-)
> 
> 
> guest-common:
> 
> Fabian Ebner (3):
>   vzdump: command line: refactor handling prune-backups
>   vzdump: command line: make sure mailto is comma-separated
>   vzdump: mailto: use email-or-username-list format
> 
>  PVE/VZDump/Common.pm | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> 
> manager:
> 
> Fabian Ebner (3):
>   vzdump: refactor parsing mailto so it can be mocked
>   test: vzdump: add tests for mailto
>   test: vzdump: rename vzdump_new_retention_test.pl to
>     vzdump_new_test.pl
> 
>  PVE/API2/VZDump.pm                            |  11 +-
>  PVE/VZDump.pm                                 |  21 +++
>  test/Makefile                                 |   8 +-
>  ...w_retention_test.pl => vzdump_new_test.pl} | 174 +++++++++++++++++-
>  4 files changed, 198 insertions(+), 16 deletions(-)
>  rename test/{vzdump_new_retention_test.pl => vzdump_new_test.pl} (74%)
> 



applied, with Dominik's R-b tag added to most (I forgot in guest-common)...
Thanks to both of you!