Page MenuHomeFreeBSD

pf: NAT rule logs its own NAT action, not PF_PASS
Needs ReviewPublic

Authored by franco_opnsense.org on Wed, Nov 27, 9:28 AM.
Tags
None
Referenced Files
F104769257: D47777.diff
Mon, Dec 9, 12:56 PM
Unknown Object (File)
Wed, Dec 4, 7:13 AM
Unknown Object (File)
Sat, Nov 30, 10:10 PM
Unknown Object (File)
Wed, Nov 27, 12:23 PM

Details

Reviewers
kp
Summary

Prior to 948e8413 the NAT log was implicitly
using r->action, but it was changed to an explicit
PF_PASS. Restore the correct behaviour by
explicitly passing nr->action.

Add a test for it while here.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 60807
Build 57691: arc lint + arc unit

Event Timeline

Shall we finish D47658 before we move on to other issues?

D47658 is on hold as I struggle to understand why the code discussed there is not covered by existing tests and how to actually trigger it (it should easily panic after all but the code is never hit).

D47658 is on hold as I struggle to understand why the code discussed there is not covered by existing tests and how to actually trigger it (it should easily panic after all but the code is never hit).

Then please update that review to state that there.

As for this one, let's start with the commit message. Try to explain both the problem and then justify chosen solution. 'fix regression' does not enlighten.

franco_opnsense.org retitled this revision from pf: fix NAT pflog regression in 948e8413 to pf: NAT rule logs it own NAT action, not PF_PASS.Wed, Nov 27, 10:27 AM
franco_opnsense.org edited the summary of this revision. (Show Details)

I changed the summary.

I'll gladly update the other review and I also appreciate a response to my last comment there.

I changed the summary.

Please do try to *explain* the problem. "Fixes a regression" does not do this.

Feel free to make a different suggestion. I've long been under the impressions explaining one liners is not the scope of a commit message and the tests should make this rather clear as requested in previous discussions.

franco_opnsense.org retitled this revision from pf: NAT rule logs it own NAT action, not PF_PASS to pf: NAT rule logs its own NAT action, not PF_PASS.Wed, Nov 27, 10:54 AM

What's the status now? From my point of you I've explained the previous behaviour, the change that caused this to exhibit the wrong behaviour and how it was fixed. I'd like to get to a more productive cooperation based on technical urgency. I don't think tests prevented this from happening and I don't think the issue gets any better discussing commit messages.