On Fri, Oct 31, 2025 at 01:05:44PM +0100, Dion Bosschieter wrote:
> Change the nwfilter driver loading mechanism to read from nwfilter.conf.
> By default, it will use the existing ebiptables driver, which can be
> replaced in the future to remove the {eb,ip}tables dependency.
>
> Added nftables to *filter_tech_drivers as an available driver option
> for users to choose from.
>
> Signed-off-by: Dion Bosschieter <[email protected]>
> ---
> src/nwfilter/nwfilter_driver.c | 49 +++++++++++++++++++--
> src/nwfilter/nwfilter_gentech_driver.c | 60 +++++++++++---------------
> src/nwfilter/nwfilter_gentech_driver.h | 4 +-
> 3 files changed, 73 insertions(+), 40 deletions(-)
Adding a config file makes sense, but we need todo a bit more.
The default file should be in git as src/nwfilter/nwfilter.conf.in
so we show (& thus document) the available config options, and
installed by meson.
Also there needs to be augeas files for parsing the config file.
For a trivial example, follow what's done for the virtual network
driver with src/network/libvirtd_network.aug and
src/network/test_libvirtd_network.aug.in. Also see the meson
file for how to invoke the test file as part of unit tests.
>
> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> index 522cfda022..18d322574d 100644
> --- a/src/nwfilter/nwfilter_driver.c
> +++ b/src/nwfilter/nwfilter_driver.c
> @@ -26,9 +26,7 @@
>
> #include "virgdbus.h"
> #include "virlog.h"
> -
> #include "internal.h"
> -
> #include "virerror.h"
> #include "datatypes.h"
> #include "nwfilter_driver.h"
> @@ -36,7 +34,6 @@
> #include "configmake.h"
> #include "virpidfile.h"
> #include "viraccessapicheck.h"
> -
> #include "nwfilter_ipaddrmap.h"
> #include "nwfilter_dhcpsnoop.h"
> #include "nwfilter_learnipaddr.h"
> @@ -203,6 +200,41 @@ nwfilterStateCleanup(void)
> }
>
>
> +/**
> + * virNWFilterLoadGentechDriverFromConfig:
> + *
> + * Loading driver name from nwfilter.conf config file
> + */
> +static char *
> +virNWFilterLoadGentechDriverFromConfig(const char *configfile)
> +{
> + g_autoptr(virConf) conf = NULL;
> + g_autofree char *drivername = NULL;
> +
> + if (access(configfile, R_OK) == 0) {
> +
> + conf = virConfReadFile(configfile, 0);
> + if (!conf)
> + return NULL;
> +
> + if (virConfGetValueString(conf, "driver", &drivername) < 0)
> + return NULL;
> +
> + if (drivername) {
> + VIR_DEBUG("nwfilter driver setting requested from config file
> %s: '%s'",
> + configfile, drivername);
> + }
> + }
> +
> + if (!drivername) {
> + drivername = g_strdup(NWFILTER_DEFAULT_DRIVER);
> + }
Rather than harcoding this, we should try to auto-detect the right
option, and we should honour the existing 'firewall_backend_priority'
meson option that is used for the network driver.
Take a look at src/network/bridge_driver_conf.c which can be used
as a template for the nwfilter driver - worth putting it into
separate nwfilter_driver_conf.{h,c} files to match the common
pattern we use in other drivers.
> +
> +
> + return g_steal_pointer(&drivername);
> +}
> +
> +
> /**
> * nwfilterStateInitialize:
> *
> @@ -217,6 +249,8 @@ nwfilterStateInitialize(bool privileged,
> {
> VIR_LOCK_GUARD lock = virLockGuardLock(&driverMutex);
> GDBusConnection *sysbus = NULL;
> + g_autofree char *configfile = NULL;
> + char *gentechdrivername = NULL;
>
> if (root != NULL) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> @@ -266,7 +300,14 @@ nwfilterStateInitialize(bool privileged,
> if (virNWFilterDHCPSnoopInit() < 0)
> goto error;
>
> - if (virNWFilterTechDriversInit(privileged) < 0)
> + configfile = g_strdup(SYSCONFDIR "/libvirt/nwfilter.conf");
> +
> + /* get chosen driver from config file */
> + gentechdrivername = virNWFilterLoadGentechDriverFromConfig(configfile);
> + if (gentechdrivername == NULL)
> + goto error;
> +
> + if (virNWFilterTechDriversInit(privileged, gentechdrivername) < 0)
> goto error;
>
> if (virNWFilterConfLayerInit(virNWFilterTriggerRebuildImpl, driver) < 0)
> diff --git a/src/nwfilter/nwfilter_gentech_driver.c
> b/src/nwfilter/nwfilter_gentech_driver.c
> index 1465734a54..adb96acca6 100644
> --- a/src/nwfilter/nwfilter_gentech_driver.c
> +++ b/src/nwfilter/nwfilter_gentech_driver.c
> @@ -32,6 +32,7 @@
> #include "nwfilter_dhcpsnoop.h"
> #include "nwfilter_ipaddrmap.h"
> #include "nwfilter_learnipaddr.h"
> +#include "nwfilter_nftables_driver.h"
> #include "virnetdev.h"
>
> #define VIR_FROM_THIS VIR_FROM_NWFILTER
> @@ -48,18 +49,20 @@ static int _virNWFilterTeardownFilter(const char *ifname);
>
> static virNWFilterTechDriver *filter_tech_drivers[] = {
> &ebiptables_driver,
> - NULL
> + &nftables_driver,
> };
>
> -int virNWFilterTechDriversInit(bool privileged)
> +int virNWFilterTechDriversInit(bool privileged, const char *drivername)
> {
> size_t i = 0;
> - VIR_DEBUG("Initializing NWFilter technology drivers");
> - while (filter_tech_drivers[i]) {
> - if (!(filter_tech_drivers[i]->flags & TECHDRV_FLAG_INITIALIZED))
> + VIR_DEBUG("Initializing NWFilter technology drivers, chosen %s",
> drivername);
> +
> + for (i = 0; i < G_N_ELEMENTS(filter_tech_drivers); i++) {
> + if (!(filter_tech_drivers[i]->flags & TECHDRV_FLAG_INITIALIZED)
> + && STREQ(filter_tech_drivers[i]->name, drivername))
> filter_tech_drivers[i]->init(privileged);
> - i++;
> }
> +
> return 0;
> }
>
> @@ -67,25 +70,20 @@ int virNWFilterTechDriversInit(bool privileged)
> void virNWFilterTechDriversShutdown(void)
> {
> size_t i = 0;
> - while (filter_tech_drivers[i]) {
> + for (i = 0; i < G_N_ELEMENTS(filter_tech_drivers); i++) {
> if ((filter_tech_drivers[i]->flags & TECHDRV_FLAG_INITIALIZED))
> filter_tech_drivers[i]->shutdown();
> - i++;
> }
> }
>
>
> static virNWFilterTechDriver *
> -virNWFilterTechDriverForName(const char *name)
> +virNWFilterInitializedTechDriver(void)
> {
> size_t i = 0;
> - while (filter_tech_drivers[i]) {
> - if (STREQ(filter_tech_drivers[i]->name, name)) {
> - if ((filter_tech_drivers[i]->flags & TECHDRV_FLAG_INITIALIZED)
> == 0)
> - break;
> + for (i = 0; i < G_N_ELEMENTS(filter_tech_drivers); i++) {
> + if ((filter_tech_drivers[i]->flags & TECHDRV_FLAG_INITIALIZED))
> return filter_tech_drivers[i];
> - }
> - i++;
> }
> return NULL;
> }
> @@ -617,7 +615,6 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverState
> *driver,
> bool *foundNewFilter)
> {
> int rc = -1;
> - const char *drvname = EBIPTABLES_DRIVER_ID;
> virNWFilterTechDriver *techdriver;
> virNWFilterObj *obj;
> virNWFilterDef *filter;
> @@ -625,12 +622,11 @@
> virNWFilterInstantiateFilterUpdate(virNWFilterDriverState *driver,
> char vmmacaddr[VIR_MAC_STRING_BUFLEN] = {0};
> virNWFilterVarValue *ipaddr;
>
> - techdriver = virNWFilterTechDriverForName(drvname);
> + techdriver = virNWFilterInitializedTechDriver();
>
> if (!techdriver) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Could not get access to ACL tech driver '%1$s'"),
> - drvname);
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Could not get access to ACL tech driver"));
> return -1;
> }
>
> @@ -768,15 +764,13 @@
> virNWFilterUpdateInstantiateFilter(virNWFilterDriverState *driver,
> static int
> virNWFilterRollbackUpdateFilter(virNWFilterBindingDef *binding)
> {
> - const char *drvname = EBIPTABLES_DRIVER_ID;
> int ifindex;
> virNWFilterTechDriver *techdriver;
>
> - techdriver = virNWFilterTechDriverForName(drvname);
> + techdriver = virNWFilterInitializedTechDriver();
> if (!techdriver) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Could not get access to ACL tech driver '%1$s'"),
> - drvname);
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Could not get access to ACL tech driver"));
> return -1;
> }
>
> @@ -793,15 +787,13 @@ virNWFilterRollbackUpdateFilter(virNWFilterBindingDef
> *binding)
> static int
> virNWFilterTearOldFilter(virNWFilterBindingDef *binding)
> {
> - const char *drvname = EBIPTABLES_DRIVER_ID;
> int ifindex;
> virNWFilterTechDriver *techdriver;
>
> - techdriver = virNWFilterTechDriverForName(drvname);
> + techdriver = virNWFilterInitializedTechDriver();
> if (!techdriver) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Could not get access to ACL tech driver '%1$s'"),
> - drvname);
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Could not get access to ACL tech driver"));
> return -1;
> }
>
> @@ -818,14 +810,12 @@ virNWFilterTearOldFilter(virNWFilterBindingDef *binding)
> static int
> _virNWFilterTeardownFilter(const char *ifname)
> {
> - const char *drvname = EBIPTABLES_DRIVER_ID;
> virNWFilterTechDriver *techdriver;
> - techdriver = virNWFilterTechDriverForName(drvname);
> + techdriver = virNWFilterInitializedTechDriver();
>
> if (!techdriver) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Could not get access to ACL tech driver '%1$s'"),
> - drvname);
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Could not get access to ACL tech driver"));
> return -1;
> }
>
> diff --git a/src/nwfilter/nwfilter_gentech_driver.h
> b/src/nwfilter/nwfilter_gentech_driver.h
> index 946d5d3d56..8f6f4d164a 100644
> --- a/src/nwfilter/nwfilter_gentech_driver.h
> +++ b/src/nwfilter/nwfilter_gentech_driver.h
> @@ -25,7 +25,9 @@
> #include "virnwfilterobj.h"
> #include "virnwfilterbindingdef.h"
>
> -int virNWFilterTechDriversInit(bool privileged);
> +#define NWFILTER_DEFAULT_DRIVER "ebiptables"
> +
> +int virNWFilterTechDriversInit(bool privileged, const char *drivername);
> void virNWFilterTechDriversShutdown(void);
>
> enum instCase {
> --
> 2.43.0
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
