On 26/11/2021 17:52, Robert Elz wrote:
     Date:        Fri, 26 Nov 2021 13:11:36 +0000
     From:        "Stephen Borrill" <sborr...@netbsd.org>
     Message-ID:  <20211126131136.63fabf...@cvs.netbsd.org>

   | Load rc configuration based on rcvar, not name, so that correct settings
   | in /etc/rc.conf.d are loaded.

This looks wrong to me (and a pullup request so soon after making a
change, before it has had any time for testing in HEAD is a *really*
bad idea).

   | Usually this does not matter as rcvar and name are set to the same value.
   | For pf_boot and npf_boot, rcvar is set to pf and npf respectively.
   |
   | Prior to the change, if:
   | rc.conf contains npf=YES
   | rc.conf.d/npf does not exist

Nor should it, that's not the file that is supposed to be used.

In rc.conf(5):


     rc.d(8) scripts that use load_rc_config from rc.subr(8) also support
     sourcing an optional end-user provided per-script override file
     /etc/rc.conf.d/service, (where service is the contents of the name
     variable in the rc.d(8) script).

That is, what should happen to make this work...

   | If:
   | rc.conf contains npf=NO (or is not set)
   | rc.conf.d/npf contains npf=YES

is that rc.conf.d/npf_boot should contain npf=YES

The rc.conf.d files have the same names as the rc.d/script files in
general, for good reason, as while they often only contain this
rcvar setting, they can contain overrides to anything in the script.
Further, if there is more than one rcvar in a script (which I think
has happened once or twice) the settings for both of them would go
in the same file, not one file for each of them.

   | This means that in the latter case, at boot time the npfctl start command
   | is never run and the firewall is not operational.

Because of user error.

Please revert this change, and request the pullup be undone as well.
I don't think it is as simple as this in the case of npf and pf. What has happened here is that the functionality of /etc/rc.d/npf has effectively been split into two parts (one to run early, the other late). It does not make sense to run one without the other. It's not like nfs which is a stack of mountd, rpcbind, nfsd, etc. which may well want different flags for each.

In rc.conf, npf=YES is sufficient, but you are advocating the setting needs to be duplicated if put into rc.conf.d. When upgrading from 8 to 9, this is a breaking change.

I propose an alternative fix which is to change /etc/rc.d/npf_boot to read:

load_rc_config $name
load_rc_config_var npf npf

--
Stephen

Reply via email to