Whether or not pflogd is configured in the /etc/rc.conf on a system, the /etc/rc.d/pflog script always emits the line "starting pflogd: " Really, this should only be output if $flog_instances is not empty.
Created attachment 155192 [details] pflog patch
Thank for opening this. There is a bit more to the issue than just printing the output you noted. For one, it breaks Puppet because Puppet is removing the leading '#' markers and gets mixed up when it sees "Starting pflog" (ref [1] lines 22-28). I saw this a while ago and commented the 'echo' line out out but seeing the PR on the mailing list reminded me I needed to to make a real patch. Sorry for not opening this sooner, but it's still plenty early enough to get this in for 10.2-RELEASE and 11.0-RELEASE. The multiple instance support was implemented in an earlier PR [2] and I just updated that PR with a request for the submitter and committer to review this PR. The fixed version is available with: fetch -o /etc/rc.d/pflog https://raw.githubusercontent.com/junovitch/my-freebsd-build/master/patches/PR199150.pflog Test cases that show the expected output from the various `service pflog <command>` commands are below. ===== EXAMPLE OF PUPPET ISSUE ===== root@myrtr:/etc/rc.d # puppet agent --test ... Error: /Stage[main]/Soekris::Service::Pf/Service[pflog]: Could not evaluate: rcvar value is empty ===== POST-PATCH MULTIPLE PFLOG CONFIGURATION ===== /etc/rc.conf pflog_enable="YES" /etc/rc.conf.d/pflog pflog_instances="pflog0 pflog1 pflog2" pflog_pflog0_dev="pflog0" pflog_pflog0_logfile="/var/log/pflog0" pflog_pflog1_dev="pflog1" pflog_pflog1_logfile="/var/log/pflog1" pflog_pflog2_dev="pflog2" pflog_pflog2_logfile="/var/log/pflog2" ===== POST-PATCH MULTIPLE PFLOG TEST CASES ===== root@myrtr:/etc/rc.d # service pflog start Starting pflog0. Starting pflog1. Starting pflog2. root@myrtr:/etc/rc.d # service pflog restart Stopping pflog0. Waiting for PIDS: 55175. Starting pflog0. Stopping pflog1. Waiting for PIDS: 55826. Starting pflog1. Stopping pflog2. Waiting for PIDS: 56253. Starting pflog2. root@myrtr:/etc/rc.d # service pflog rcvar # pflog0 # pflog_enable="YES" # (default: "") # pflog1 # pflog_enable="YES" # (default: "") # pflog2 # pflog_enable="YES" # (default: "") root@myrtr:/etc/rc.d # service pflog enabled root@myrtr:/etc/rc.d # service pflog reload root@myrtr:/etc/rc.d # service pflog resync # (All display no output) root@myrtr:/etc/rc.d # service pflog status pflog0 is running as pid 57645. pflog1 is running as pid 58831. pflog2 is running as pid 60086. root@myrtr:/etc/rc.d # service pflog poll Waiting for PIDS: 57645 # ... root@myrtr:/etc/rc.d # service pflog stop Stopping pflog0. Waiting for PIDS: 57645. Stopping pflog1. Waiting for PIDS: 58831. Stopping pflog2. Waiting for PIDS: 60086. ===== POST-PATCH SINGLE PFLOG CONFIGURATION ===== /etc/rc.conf pflog_enable="YES" ===== POST-PATCH SINGLE PFLOG TEST CASES ===== root@myrtr:/etc/rc.d # service pflog start Starting pflog. root@myrtr:/etc/rc.d # service pflog restart Stopping pflog. Waiting for PIDS: 71252. Starting pflog. root@myrtr:/etc/rc.d # service pflog rcvar # pflog # pflog_enable="YES" # (default: "") root@myrtr:/etc/rc.d # service pflog enabled root@myrtr:/etc/rc.d # service pflog reload root@myrtr:/etc/rc.d # service pflog resync # (All display no output) root@myrtr:/etc/rc.d # service pflog status pflog is running as pid 72503. root@myrtr:/etc/rc.d # service pflog poll Waiting for PIDS: 72503 # ... root@myrtr:/etc/rc.d # service pflog stop Stopping pflog. Waiting for PIDS: 72503. ===== REFERENCES ===== [1] https://github.com/puppetlabs/puppet/blob/cb4ad1f19b043a70ba17e4103a25f5c97a76948b/lib/puppet/provider/service/freebsd.rb [2] https://bugs.freebsd.org/158171
I have a couple of questions. 1) Is my assumption that this a typo correct? -# REQUIRE: FILESYSTEMS netif +# REQUIRE: FILESYSTEMS netif FILESYSTEMS 2) Could you explain this to me? Why does this need to be recursive here? - run_rc_command "$1" + /etc/rc.d/pflog $1 $i
Created attachment 155195 [details] Nonrecursive Pflog -- Broken in restart case 2. The recursive call case's justification is best shown with the restart command (shown below). I've attached my original diff that kept the non-recursive behavior as a demo but the only way to make all rc commands work is by removing the comment on _rc_restart_done=false shown in the patch. Line 1102 of /etc/rc.subr explains that gets set to prevent being called more than once by any given script. So that works... but working and right thing to do aren't necessarily the same thing. The recursive call gives the ability to do individual devices via `service pflog restart pflog1`. This is similar in concept to /etc/rc.d/wpa_supplicant using the $2 for interface and it's similar in implementation to /etc/rc.d/netif looping through each interface and calling `/etc/rc.d/routing start any $_if`. Finally, it's worth mentioning that `service pflog restart` is broken in the current version for the reason above. ===== CURRENT PFLOG ===== root@myrtr:/etc/rc.d # service pflog restart Starting pflogd: pflog0 pflog1 pflog2 Stopping pflog. Waiting for PIDS: 16977. Starting pflog. ===== ATTACHED PFLOG ===== root@myrtr:/etc/rc.d # service pflog restart Stopping pflog0. Waiting for PIDS: 52490. Starting pflog0. ===== ATTACHED PFLOG, comment removed on _rc_restart_done=false ===== root@myrtr:/etc/rc.d # service pflog restart Stopping pflog0. Waiting for PIDS: 55852. Starting pflog0. Stopping pflog1. Waiting for PIDS: 12762. Starting pflog1. Stopping pflog2. Waiting for PIDS: 13809. Starting pflog2.
(In reply to Josh Paetzel from comment #3) 2 was answered above. 1. The Github link to the full patch was mainly for the PR submitter's benefit. It was made against 10-STABLE so has the duplicate FILESYSTEMS require that already was fixed in HEAD in r275324.
Created attachment 155198 [details] pflog patch Extremely minor nitpick regarding using export. - export name=$pflog_dev + name=$pflog_dev
A commit references this bug: Author: jpaetzel Date: Sun Apr 5 17:09:59 UTC 2015 New revision: 281112 URL: https://svnweb.freebsd.org/changeset/base/281112 Log: Bug fixes and feature adds - Remove extranious echo that breaks puppet - Handle restarts of multiple pflog devices correctly - Add the ability to perform actions on specific pflog devices. PR: 199150 Submitted by: jason.unovitch@gmail.com MFC after: 3 days Changes: head/etc/rc.d/pflog
Committed, thanks!
A commit references this bug: Author: jpaetzel Date: Sun Apr 12 01:14:44 UTC 2015 New revision: 281446 URL: https://svnweb.freebsd.org/changeset/base/281446 Log: MFC 281112, 281166 Bug fixes and feature adds - Remove extranious echo that breaks puppet - Handle restarts of multiple pflog devices correctly - Add the ability to perform actions on specific pflog devices. Typo Fix. PR: 199150 Changes: _U stable/10/ stable/10/etc/rc.d/pflog