Re: [PATCH v2 10/27] util/network: new virFirewallBackend enum

On Sun, Apr 21, 2024 at 10:53:18PM -0400, Laine Stump wrote: > (This paragraph is for historical reference only, described only to > avoid confusion of past use of the name with its new use) In a past > life, virFirewallBackend had been a private static in virfirewall.c > that was set at daemon init time, and used to globally (i.e. for all > drivers in the daemon) determine whether to directly execute iptables > commands, or to run them indirectly via the firewalld passthrough > API. This was removed in commit d566cc55, since we decided that using > the firewalld passthrough API is never appropriate. > > Now the same enum, virFirewallBackend, is being reintroduced, with a > different meaning and usage pattern. It will be used to pick between > using nftables commands or iptables commands (in either case directly > handled by libvirt, *not* via firewalld). Additionally, rather than > being a static known only within virfirewall.c and applying to all > firewall commands for all drivers, each virFirewall object will have > its own backend setting, which will be set during virFirewallNew() by > the driver who wants to add a firewall rule. > > This will allow the nwfilter and network drivers to each have their > own backend setting, even when they coexist in a single unified > daemon. At least as important as that, it will also allow an instance > of the network driver to remove iptables rules that had been added by > a previous instance, and then add nftables rules for the new instance > (in the case that an admin, or possibly an update, switches the driver > backend from iptables to nftable) > > Initially, the enum will only have one usable value - > VIR_FIREWALL_BACKEND_IPTABLES, and that will be hardcoded into all > calls to virFirewallNew(). The other enum value (along with a method > of setting it for each driver) will be added later, when it can be > used (when the nftables backend is in the code). > > Signed-off-by: Laine Stump <laine(a)redhat.com&gt; > --- > src/libvirt_private.syms | 3 +++ > src/network/network_iptables.c | 6 +++--- > src/nwfilter/nwfilter_ebiptables_driver.c | 16 ++++++++-------- > src/util/virebtables.c | 4 ++-- > src/util/virfirewall.c | 16 +++++++++++++++- > src/util/virfirewall.h | 12 +++++++++++- > tests/virfirewalltest.c | 20 ++++++++++---------- > 7 files changed, 52 insertions(+), 25 deletions(-) > > diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c > index 56d43bfdde..489482fd83 100644 > --- a/src/util/virfirewall.c > +++ b/src/util/virfirewall.c > @@ -35,6 +35,11 @@ > > VIR_LOG_INIT("util.firewall"); > > +VIR_ENUM_IMPL(virFirewallBackend, > + VIR_FIREWALL_BACKEND_LAST, > + "UNSET", /* not yet set */ > + "iptables"); > + snip > diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h > index 956bf0e2bf..7f0fee047d 100644 > --- a/src/util/virfirewall.h > +++ b/src/util/virfirewall.h > @@ -34,9 +35,18 @@ typedef enum { > VIR_FIREWALL_LAYER_LAST, > } virFirewallLayer; > > -virFirewall *virFirewallNew(void); > +typedef enum { > + VIR_FIREWALL_BACKEND_UNSET, > + VIR_FIREWALL_BACKEND_IPTABLES, > + > + VIR_FIREWALL_BACKEND_LAST, > +} virFirewallBackend; We're forcing the callers to all pass VIR_FIREWALL_BACKEND_IPTABLES,

so I'm wondering if we actually need to have VIR_FIREWALL_BACKEND_UNSET at all ? If we eliminate it, then that removes need to check for this illegal value and report errors I guess. With regards, Daniel