-
Notifications
You must be signed in to change notification settings - Fork 581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ARPwatch improvements. Issue #10770 #912
Conversation
if (!empty($notifications_recipient)) { | ||
$rc['start'] .= ' -w '.escapeshellarg($notifications_recipient); | ||
} | ||
$rc['start'] .= "\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines do not need to be indented the extra spaces here. They should be aligned with line 60. Lines 61-66 are indented more as they are continuations of the statement on line 60.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -228,7 +232,7 @@ $fd = fopen('php://stdin','r'); | |||
$message = stream_get_contents($fd); | |||
fclose($fd); | |||
|
|||
if (false !== $message) { | |||
if ((false !== $message) && (false === strpos($message, ': Cron '))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be done unconditionally in this way? The feature request mentioned adding a checkbox/option for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checkbox added, but this is weird behavior anyway
https://redmine.pfsense.org/issues/8454
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, since arpwatch can only use /usr/sbin/sendmail
to send mail, anything else that wants /usr/sbin/sendmail
will also take advantage of it being there (Notably Cron). There isn't really a good way around that as far as I'm aware. Unless the script can check something in its environment to tell it's being called by arpwatch specifically and nothing else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be OK with this Disable Cron emails
checkbox for now
c752539
to
32b2d60
Compare
Redmine Issue: https://redmine.pfsense.org/issues/10770
Ready for review
Allow to disable email notifications by removing recipient email https://redmine.pfsense.org/issues/10770
Do not send cron email notifications https://redmine.pfsense.org/issues/10771
Do not show PHP error if
ethercodes.dat
file is not found:[22-Jul-2020 10:11:21 Europe/Moscow] PHP Warning: file_get_contents(/usr/local/arpwatch/ethercodes.dat): failed to open stream: No such file or directory in /usr/local/pkg/arpwatch.inc on line 170