Page MenuHomeFreeBSD

pf: Fix max-src-conn when rules are added via netlink
ClosedPublic

Authored by vegeta_tuxpowered.net on Thu, Sep 26, 4:07 PM.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

vegeta_tuxpowered.net added inline comments.
netpfil/pf/pf_nl.h
261 ↗(On Diff #143756)

Is the Netlink interface considered "stable"? If not, we could move this constant to its rightful place instead appending it here.

netpfil/pf/pf_nl.h
261 ↗(On Diff #143756)

It's a user-facing ABI, yes. We can't easily change it.

We could maybe get away with it here, because I believe this code is only in main so far, but I'm not sure it's worth it just for an order that'll we'll lose in due course anyway (as we add features, which is pretty likely in the rule structure.

Thanks for spotting (and fixing) this.

One small issue though: all of your recent patches seem to end up moving files. This one moves libpfctl/libpfctl/libpfctl.c to libpfctl/libpfctl.c and sys/netpfil/pf/pf_nl.* to netpfil/pf/pf_nl.*.
I'll fix these manually before I push them, but can you take a look at how you generate future ones?

It may be worth testing tools/tools/git/git-arc.sh for this. It's what I use to pick commits out of Phabricator into my local git tree and to post local git commits to Phabricator. It makes the round trip a lot less painful.

In D46797#1067141, @kp wrote:

Thanks for spotting (and fixing) this.

One small issue though: all of your recent patches seem to end up moving files. This one moves libpfctl/libpfctl/libpfctl.c to libpfctl/libpfctl.c and sys/netpfil/pf/pf_nl.* to netpfil/pf/pf_nl.*.
I'll fix these manually before I push them, but can you take a look at how you generate future ones?

It may be worth testing tools/tools/git/git-arc.sh for this. It's what I use to pick commits out of Phabricator into my local git tree and to post local git commits to Phabricator. It makes the round trip a lot less painful.

I don't see the file location change in my diff. I use the same method I've always did. The location change seems to be present only in my most recent reviews. Maybe there's something wrong in what I click in the Phabricator's web interface. I'll investigate.

[11:19:25] MB-20001482 freebsd-src/ kajetan_src_node_cleanup # git diff --no-prefix -U999999 9deb6bfb146c~1 9deb6bfb146c > ~/workspace/freebsd-patches/a.diff
[11:20:38] MB-20001482 freebsd-src/ kajetan_src_node_cleanup # head ~/workspace/freebsd-patches/a.diff
diff --git lib/libpfctl/libpfctl.c lib/libpfctl/libpfctl.c
index c8eeb913e912..36b83c480aa7 100644
--- lib/libpfctl/libpfctl.c
+++ lib/libpfctl/libpfctl.c
@@ -1,2916 +1,2918 @@
 /*-
  * SPDX-License-Identifier: BSD-2-Clause
  *
  * Copyright (c) 2021 Rubicon Communications, LLC (Netgate)
  * All rights reserved.
netpfil/pf/pf_nl.h
261 ↗(On Diff #143756)

but I'm not sure it's worth it just for an order that'll we'll lose in due course anyway

Ok, then let's merge it as it is.

This revision was not accepted when it landed; it landed in state Needs Review.Fri, Sep 27, 12:30 PM
This revision was automatically updated to reflect the committed changes.