FreeBSD's PF has a new interface so leverage libpfctl to access it so… #4148

brd · 2023-11-07T13:57:23Z

… the right interface is used depending on the version

I am interested in feedback if there is possibly a better way to do this.

ChangeLog: pf plugin: Support for libpfctl has been added.

octo

Hi @brd, thanks for the contribution!

My overall concern is that this is making the code more complicated but doesn't provide additional utility. Is there anything that this new interface allows us to do that we were unable to do previously? If not, I think it would be preferable to keep the code simple.

src/pf.c

brd · 2023-11-24T21:39:07Z

OK, I have split it into two separate functions, let me know what you think!

octo · 2023-11-24T22:48:38Z

This looks much better now, thank you! I'm still not quite clean on why we need special code for FreeBSD if is does the exact same as the generic code. What's the benefit?

brd · 2023-11-27T20:06:31Z

libpfctl is only for FreeBSD, as the interface to access pf info is changing over time.. it was ioctl, then nvlist, and now it is netlink. libpfctl abstracts that away so collectd does not need to have ifdef for the different versions of FreeBSD.

octo · 2023-11-28T09:21:55Z

Thanks for the feedback. That does indeed sound like using an abstraction like libpfctl is useful.

The FreeBSD build currently fails with:

src/pf.c:39:10: fatal error: 'libpfctl.h' file not found
#include <libpfctl.h>
         ^~~~~~~~~~~~
1 error generated.

This is because:

Our FreeBSD build instance does not yet have the libpfctl port installed. You can fix this by adding to the pkg_install_script step in .cirrus.yml.
We need a check for this header file in the configure script. Search for net/pfvar.h in configure.ac to see how the decision to build the "pf" plugin is made right now. Then use HAVE_LIBPFCTL_H instead of __FreeBSD__ to chose which implementation to use.

I anticipate that the next issue is going to be a linker error because we need to link with libpfctl. This requires a check for the library with AC_CHECK_LIB in configure.ac and changes to the pf_la_LDFLAGS in Makefile.am.

octo · 2023-12-21T20:22:55Z

Hi @brd, do you need any help making the above changes?

brd · 2024-02-03T19:49:36Z

@octo sorry for the delay, I kept meaning to get back to this and kept forgetting!

I am struggling with getting the configure script to work.. I tried this:

diff --git a/Makefile.am b/Makefile.am
index 1d3c69f2..71821bd0 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1744,7 +1744,7 @@ endif
 if BUILD_PLUGIN_PF
 pkglib_LTLIBRARIES += pf.la
 pf_la_SOURCES = src/pf.c
-pf_la_LDFLAGS = $(PLUGIN_LDFLAGS)
+pf_la_LDFLAGS = $(PLUGIN_LDFLAGS) $(PF_LDFLAGS)
 endif
 
 if BUILD_PLUGIN_PINBA
diff --git a/configure.ac b/configure.ac
index 5383e3e2..f95fa1a4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -693,6 +693,9 @@ AC_CHECK_HEADERS([net/pfvar.h],
     # include <netinet/in.h>
     #endif
   ]]
+  AC_CHECK_LIB([libpfctl], [pfctl_status_counter],
+    PF_LDFLAGS="$PF_LDFLAGS -lpfctl"
+  )
 )

octo · 2024-02-03T21:47:06Z

Hey @brd, no worries. I think the following should work. Note: it's untested!

diff --git a/Makefile.am b/Makefile.am
index 64c719c48..6b0e4b4ea 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1761,6 +1761,9 @@ if BUILD_PLUGIN_PF
 pkglib_LTLIBRARIES += pf.la
 pf_la_SOURCES = src/pf.c
 pf_la_LDFLAGS = $(PLUGIN_LDFLAGS)
+if BUILD_WITH_LIBPFCTL
+pf_la_LDFLAGS += -lpfctl
+endif
 endif

 if BUILD_PLUGIN_PINBA
diff --git a/configure.ac b/configure.ac
index dad27f53b..ae77bf3b5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2040,6 +2040,13 @@ if test "x$with_kvm_openfiles" = "xyes"; then
   with_libkvm="yes"
 fi

+AC_CHECK_HEADERS([libpfctl.h])
+AC_CHECK_LIB([libpfctl], [pfctl_status_counter],
+  [with_libpfctl="yes"],
+  [with_libpfctl="no"]
+)
+AM_CONDITIONAL([BUILD_WITH_LIBPFCTL], [test "x$with_libpfctl" = "xyes"])
+
 # --with-cuda {{{
 AC_ARG_WITH([cuda],
   [AS_HELP_STRING([--with-cuda@<:@=PREFIX@:>@], [Path to cuda.])],
diff --git a/src/pf.c b/src/pf.c
index 9681d366a..2dbcd31cc 100644
--- a/src/pf.c
+++ b/src/pf.c
@@ -36,6 +36,10 @@

 #include <net/pfvar.h>

+#if HAVE_LIBPFCTL_H
+#include <libpfctl.h>
+#endif
+
 #ifndef FCNT_NAMES
 #if FCNT_MAX != 3
 #error "Unexpected value for FCNT_MAX"

brd · 2024-02-06T17:09:12Z

Hi @octo I think I got it working right.. have a look and let me know what you think.

octo

This looks much better, thank you @brd! Once we fix the AC_CHECK_HEADERS check this is good to go.

octo · 2024-02-06T20:12:04Z

configure.ac

+AC_CHECK_HEADERS([libpfctl.h], [], [],
+[
+  #include <sys/queue.h>
+  #include <sys/types.h>
+  #include <netinet/in.h>
+  #include <net/if.h>
+  #include <net/pfvar.h>
+])


This check is inconsequential: neither the success nor the failure case have any side effects.

Option 1: If you simplify this to

AC_CHECK_HEADERS([libpfctl.h])

then autoconf's default behavior will be active, which will create a HAVE_LIBPFCTL_H define if the header is present. You can use it instead of __FreeBSD__ in the .c file.

Option 2: In the unlikely case that you have to specify the other headers, skipping the arguments entirely will trigger the default behavior (I think):

AC_CHECK_HEADERS([libpfctl.h],,, # <-- empty arguments [ #include <sys/queue.h> #include <sys/types.h> #include <netinet/in.h> #include <net/if.h> #include <net/pfvar.h> ])

Option 3: remove entirely.

Case in point, the build currently fails with:

src/pf.c:39:10: fatal error: 'libpfctl.h' file not found #include <libpfctl.h> ^~~~~~~~~~~~ 1 error generated. --- src/pf.lo --- *** [src/pf.lo] Error code 1

This is despite the configure script technically checking for the header's existence:

checking for libpfctl.h... no checking for pfctl_status_counter in -llibpfctl... no

Hmm, the build works for me where I have libpfctl present:

checking for libpfctl.h... yes checking for pfctl_status_counter in -lpfctl... yes

I had to add the other includes so that the test worked.. but if we don't need it, I am happy to remove it.

Where are you seeing that build failure? It looks like the checks on this PR are successful?

I see some failed checks below. The detailed view of the FreeBSD one is here: https://cirrus-ci.com/task/6128106226843648

ah, thank you! I am not very familiar with the github actions and didn't see where I could get that detail.. the in github link didn't show any errors, just looked like it built successfully.

In the FreeBSD port, we set CPPFLAGS= -I/usr/local/include :
https://cgit.freebsd.org/ports/tree/net-mgmt/collectd5/Makefile#n84

Looks like that is needed in the .cirrus.yml...

hmm, that doesn't seem to be working according to the config.log

There is a typo in the flag definition (CPPLAGS instead of CPPFLAGS).

We definitely need HAVE_LIBPFCTL_H and cannot rely on __FreeBSD__, see comment below. Please try if

AC_CHECK_HEADERS([libpfctl.h])

works, that would be the easiest.

octo · 2024-02-06T20:17:26Z

src/pf.c

+  int fd;
+
+  fd = open(pf_device, O_RDONLY);


Optional coding style improvement.

Suggested change

int fd;

fd = open(pf_device, O_RDONLY);

int fd = open(pf_device, O_RDONLY);

.cirrus.yml

configure.ac

octo

The build succeeds now. The remaining issue is in src/ras.c which is unrelated.

src/pf.c

octo · 2024-02-07T16:33:47Z

configure.ac

+AC_CHECK_HEADERS([libpfctl.h], [], [],
+[
+  #include <sys/queue.h>
+  #include <sys/types.h>
+  #include <netinet/in.h>
+  #include <net/if.h>
+  #include <net/pfvar.h>
+])


We definitely need HAVE_LIBPFCTL_H and cannot rely on __FreeBSD__, see comment below. Please try if

AC_CHECK_HEADERS([libpfctl.h])

works, that would be the easiest.

… the right interface is used depending on the version

brd · 2024-02-07T17:09:23Z

I think that addresses all the build issues.. I kept the #includes for the AC_CHECK_HEADERS since the test failed for me without them.

octo

LGTM, thanks @brd!

brd · 2024-02-07T17:35:13Z

Thank you for all your help! It is much appreciated

octo · 2024-02-07T18:54:32Z

@brd You're very welcome. Thanks for your contribution!

brd requested a review from a team as a code owner November 7, 2023 13:57

octo requested changes Nov 24, 2023

View reviewed changes

src/pf.c Outdated Show resolved Hide resolved

brd force-pushed the freebsd-pf branch 2 times, most recently from 36c20ee to 9ec1d73 Compare November 24, 2023 21:38

octo added the bsd only label Nov 24, 2023

brd force-pushed the freebsd-pf branch from 9ec1d73 to 57a1b64 Compare November 27, 2023 20:38

octo added the Waiting for response - 1st time Waiting for contributor to respond - 1st call label Dec 21, 2023

brd force-pushed the freebsd-pf branch from 57a1b64 to bf6c2cc Compare February 6, 2024 17:07

octo reviewed Feb 6, 2024

View reviewed changes

brd force-pushed the freebsd-pf branch from bf6c2cc to db5c6e9 Compare February 7, 2024 15:14

octo reviewed Feb 7, 2024

View reviewed changes

.cirrus.yml Outdated Show resolved Hide resolved

brd force-pushed the freebsd-pf branch from db5c6e9 to 2c00beb Compare February 7, 2024 15:52

octo reviewed Feb 7, 2024

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

brd force-pushed the freebsd-pf branch from 2c00beb to a77e44a Compare February 7, 2024 16:20

octo requested changes Feb 7, 2024

View reviewed changes

FreeBSD's PF has a new interface so leverage libpfctl to access it so…

e469c5d

… the right interface is used depending on the version

brd force-pushed the freebsd-pf branch from a77e44a to e469c5d Compare February 7, 2024 16:57

octo added the Feature label Feb 7, 2024

octo approved these changes Feb 7, 2024

View reviewed changes

octo merged commit 9473060 into collectd:main Feb 7, 2024
23 of 24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FreeBSD's PF has a new interface so leverage libpfctl to access it so… #4148

FreeBSD's PF has a new interface so leverage libpfctl to access it so… #4148

brd commented Nov 7, 2023 •

edited by octo

octo left a comment

brd commented Nov 24, 2023

octo commented Nov 24, 2023

brd commented Nov 27, 2023

octo commented Nov 28, 2023 •

edited

octo commented Dec 21, 2023

brd commented Feb 3, 2024 •

edited

octo commented Feb 3, 2024

brd commented Feb 6, 2024

octo left a comment

octo Feb 6, 2024

octo Feb 6, 2024

brd Feb 6, 2024

octo Feb 7, 2024

brd Feb 7, 2024

brd Feb 7, 2024

octo Feb 7, 2024

octo Feb 7, 2024

octo Feb 6, 2024

octo left a comment

octo Feb 7, 2024

brd commented Feb 7, 2024

octo left a comment

brd commented Feb 7, 2024

octo commented Feb 7, 2024

FreeBSD's PF has a new interface so leverage libpfctl to access it so… #4148

FreeBSD's PF has a new interface so leverage libpfctl to access it so… #4148

Conversation

brd commented Nov 7, 2023 • edited by octo

octo left a comment

Choose a reason for hiding this comment

brd commented Nov 24, 2023

octo commented Nov 24, 2023

brd commented Nov 27, 2023

octo commented Nov 28, 2023 • edited

octo commented Dec 21, 2023

brd commented Feb 3, 2024 • edited

octo commented Feb 3, 2024

brd commented Feb 6, 2024

octo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brd commented Feb 7, 2024

octo left a comment

Choose a reason for hiding this comment

brd commented Feb 7, 2024

octo commented Feb 7, 2024

brd commented Nov 7, 2023 •

edited by octo

octo commented Nov 28, 2023 •

edited

brd commented Feb 3, 2024 •

edited