On 09/14/2017 11:15 AM, Philip Abernethy wrote:
On Wed, 2017-09-13 at 08:11 +0200, Thomas Lamprecht wrote:
On 09/08/2017 02:56 PM, Philip Abernethy wrote:
Using the nodename is not correct and can lead to mails not
forwarding
in restrictive mail server configurations.
---
src/PVE/HA/Env/PVE2.pm | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/PVE/HA/Env/PVE2.pm b/src/PVE/HA/Env/PVE2.pm
index fdfadd7..722d1b7 100644
--- a/src/PVE/HA/Env/PVE2.pm
+++ b/src/PVE/HA/Env/PVE2.pm
@@ -204,8 +204,8 @@ sub log {
sub sendmail {
my ($self, $subject, $text) = @_;
- my $mailfrom = 'root@' . $self->nodename();
- my $mailto = 'root@localhost';
+ my $mailfrom = 'root';
+ my $mailto = 'root';
needs this to change? root@localhost worked as intended not for
"mailto"?
my $mailto = 'root@localhost';:
X-original-to: root@localhost
Delivered-to: root@localhost
my $mailto = 'root';:
X-original-to: root
Delivered-to: r...@ceph2.proxmox.com
The result is the same, pvemailforward forwarding the mail to whatever
address is configured in the datacenter's user config. But I think it's
more consistent that way.
OK, so if you want this then please write it in the commit message that
you touched and changed the second value just for consistence sake not
for fixing the bug.
A comment would be nice too if we take this approach, so that the
unsuspecting reader may understand that while both values are root
they
mean something different when resolved by pvemailforward...
Fair. Can do. Although they don't really mean anything different. They
aren't even handled differently. There's simply a forwarding for mail
delivered to the root's mailbox.
Besides that looks OK.
But I'd still like to see the sendmail function a bit adapted so
that this
is possible:
sendmail($subject, text => $foo); # send to root configured email a
plain text mail with roots or dc->email_from as from
sendmail($subject, text => $foo, html => $bar); # same but also a
HTML part
sendmail($subject, to => $backup_mail, text => $foo, html => $bar); #
same as above but a custom "to" (e.g. for backups)
Seems like a nicer interface for the user to me, thoughts?
Originally I had edited PVE::Tools::sendmail to use named arguments,
but that resulted in having to touch the backup file as well. So I'd be
touching three files for barely any gain. The backup uses all fields
anyway and the ha-manager only has a single undefined in the call.
If you say you prefer named arguments, sure I can add it back in.
FYI, theres another sendmail (the binary not our function) usage in:
pve-manager/PVE/API2/APT.pm
A nicer interface is never "barely any gain" ;) But yes you are
right here, the quick'n lazy (in the good sense) fix is this.
So if you give me a comment and improved commit message I'm totally
OK with this.
PVE::Tools::sendmail($mailto, $subject, $text, undef,
$mailfrom);
}
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
