Re: [PATCH v3 12/27] network: support setting firewallBackend from network.conf

On 4/25/24 12:30 PM, Daniel P. Berrangé wrote: > On Thu, Apr 25, 2024 at 01:38:18AM -0400, Laine Stump wrote: > > It still can have only one useful value ("iptables"), but once a 2nd > > value is supported, it will be selectable by setting > > "firewall_backend=nftables" in /etc/libvirt/network.conf. > > > > If firewall_backend isn't set in network.conf, then libvirt will check > > to see if the iptables binary is present on the system and set > > firewallBackend to iptables - if no iptables binary is found, that is > > considered a fatal error (since no networks can be started anyway), so > > an error is logged and startup of the network driver fails. > > > > NB: network.conf is itself created from network.conf.in at build time, > > and the advertised default setting of firewall_backend (in a commented > > out line) is set from the meson_options.txt setting > > "firewall_backend". This way the conf file will have correct > > information no matter what default backend is chosen at build time. > > > > Signed-off-by: Laine Stump <laine(a)redhat.com&gt; > > Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; > > --- > > Changes from V2: > > > > * make failure to find iptables during network driver init fatal > > > > * recognize firewall_backend setting in meson_options.txt and > > propagate it through to: > > > > * meson-config.h (along with a numeric constant > > FIREWALL_BACKEND_DEFAULT_IPTABLES, which can be used for > > conditional compilation) > > * network.conf - default setting and comments use @FIREWALL_BACKEND@ > > * test_libvirtd_network.aug.in so that default firewall backend > > for testing is set properly (this required calisthenics similar to > > qemu_user_group_hack_conf in the qemu driver's meson.build > > (see commit 64a7b8203bf) > > > > meson.build | 5 +++ > > meson_options.txt | 1 + > > src/network/bridge_driver.c | 22 ++++++----- > > src/network/bridge_driver_conf.c | 50 ++++++++++++++++++++++++ > > src/network/bridge_driver_conf.h | 3 ++ > > src/network/bridge_driver_linux.c | 6 ++- > > src/network/bridge_driver_nop.c | 6 ++- > > src/network/bridge_driver_platform.h | 6 ++- > > src/network/libvirtd_network.aug | 5 ++- > > src/network/meson.build | 22 ++++++++++- > > src/network/network.conf.in | 8 ++++ > > src/network/test_libvirtd_network.aug.in | 3 ++ > > tests/networkxml2firewalltest.c | 2 +- > > 13 files changed, 119 insertions(+), 20 deletions(-) > > > > diff --git a/src/network/bridge_driver_conf.c b/src/network/bridge_driver_conf.c > > index a2edafa837..25cbbf8933 100644 > > --- a/src/network/bridge_driver_conf.c > > +++ b/src/network/bridge_driver_conf.c > > @@ -25,6 +25,7 @@ > > #include "datatypes.h" > > #include "virlog.h" > > #include "virerror.h" > > +#include "virfile.h" > > #include "virutil.h" > > #include "bridge_driver_conf.h" > > @@ -62,6 +63,7 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED, > > const char *filename) > > { > > g_autoptr(virConf) conf = NULL; > > + g_autofree char *firewallBackendStr = NULL; > > /* if file doesn't exist or is unreadable, ignore the "error" */ > > if (access(filename, R_OK) == -1) > > Not visible, but here we are about to do 'return 0'. > > This means that if the config file doesn't exist at all, then > none of this method below will run. > > The logic for detecting the "best" firewall backend in the > "} else {" clause below will thus never run. So without a > config file on disk, it will always implicitly get the > iptables backend set. > oops. I hadn't noticed that since I always have a config file. > We should set all defaults upfront, which means detectnig > nft vs iptables. Then set an override later when reading > the config, if it exists. Okay, how about this: 1) check for the existence of both backends and save that in a couple of bools. 2) based on the the results of (1) plus meson_options.txt (or meson commandline) setting of firewall_backend, do a preliminary setting of backend (or fail if neither is found).

3) If the config file has a firewall_backend setting, look at the results of (1) to make sure it's available, and set that (if it's *not* available, do we fail? Or ignore? I'm inclined to say fail)

> > + } else { > > + > > + /* no .conf setting, so see what this host supports by looking > > + * for binaries used by the backends, and set accordingly. > > + */ > > + g_autofree char *iptablesInPath = NULL; > > + bool binaryFound = false; > > + > > + /* virFindFileInPath() uses g_find_program_in_path(), > > + * which allows absolute paths, and verifies that > > + * the file is executable. > > + */ > > +#if defined(FIREWALL_BACKEND_DEFAULT_IPTABLES) > > Do we need this check ? It should always be set, and if not, it'll > be a compile error anyway. The way the logic in meson.build works, it looks at the setting of firewall_backend, and if that is "iptables" then it adds #define FIREWALL_BACKEND_DEFAULT_IPTABLES to meson-config.h, or if it's set to "nftables" then it adds #define FIREWALL_BACKEND_DEFAULT_NFTABLES (the name in the #elif should have been ...NFTABLES, as I discovered last night right after sending the patch mails - I hadn't received back any of the mails to be able reply to myself then, so it had to wait until this morning (I was already way too late getting to bed, so didn't want to wait around any longer :-)) This all feels rather "hacky" (both because the FIREWALL_BACKEND_DEFAULT_* constants are derived from the string setting of firewall_backend, and also because the conditionally compiled code is (short but) duplicated. But it was the simplest way I could see to get conditional compilation of the order of the checks rather than needing to do a string compare of the constant FIREWALL_BACKEND at runtime (since C preprocessor expressions can't do string compares). If someone wants to point out the obvious simple solution that I'm refusing to see, I'd be happy to do that instead! (I don't want to require the person doing the build to have to manually set more than a single option, and the name of the backend seems like the simplest)