On Thu, Apr 25, 2024 at 01:04:10PM -0400, Laine Stump wrote:
> 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@xxxxxxxxxx>
> > > Reviewed-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > > ---
> > > 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).
Yes, fail if neither is present.
>
> 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)
Yes, fail if user's request choice is not available.
> > > + } 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)
Get meson to set
#define NETWORK_FIREWALL_BACKND VIR_FIREWALL_BACKEND_IPTABLES
or
#define NETWORK_FIREWALL_BACKND VIR_FIREWALL_BACKEND_NFTABLES
so you can just use the defined value directly
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 :|
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
