Re: [PATCH 2/8] network: firewalld: add networkAddHybridFirewallDRules()

On 11/10/22 17:31, Eric Garver wrote: > This factors out the firewalld pieces of the iptables + firewalld > backend. > > Signed-off-by: Eric Garver <eric(a)garver.life&gt; > --- > src/network/bridge_driver_linux.c | 117 ++++++++++++++++-------------- > 1 file changed, 61 insertions(+), 56 deletions(-) > > diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c > index d9597d91beed..88a8e9c5fa27 100644 > --- a/src/network/bridge_driver_linux.c > +++ b/src/network/bridge_driver_linux.c > @@ -801,6 +801,58 @@ networkRemoveIPSpecificFirewallRules(virFirewall *fw, > } > > > +static int > +networkAddHybridFirewallDRules(virNetworkDef *def) > +{ > + /* if firewalld is active, try to set the "libvirt" zone. This is > + * desirable (for consistency) if firewalld is using the iptables > + * backend, but is necessary (for basic network connectivity) if > + * firewalld is using the nftables backend > + */ > + > + /* if the "libvirt" zone exists, then set it. If not, and > + * if firewalld is using the nftables backend, then we > + * need to log an error because the combination of > + * nftables + default zone means that traffic cannot be > + * forwarded (and even DHCP and DNS from guest to host > + * will probably no be permitted by the default zone > + */ > + if (virFirewallDZoneExists("libvirt")) { > + if (virFirewallDInterfaceSetZone(def->bridge, "libvirt") < 0) > + return -1; > + } else { > + unsigned long version; > + int vresult = virFirewallDGetVersion(&version); > + > + if (vresult < 0) > + return -1; > + > + /* Support for nftables backend was added in firewalld > + * 0.6.0. Support for rule priorities (required by the > + * 'libvirt' zone, which should be installed by a > + * libvirt package, *not* by firewalld) was not added > + * until firewalld 0.7.0 (unless it was backported). > + */ > + if (version >= 6000 && > + virFirewallDGetBackend() == VIR_FIREWALLD_BACKEND_NFTABLES) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("firewalld is set to use the nftables " > + "backend, but the required firewalld " > + "'libvirt' zone is missing. Either set " > + "the firewalld backend to 'iptables', or " > + "ensure that firewalld has a 'libvirt' " > + "zone by upgrading firewalld to a " > + "version supporting rule priorities " > + "(0.7.0+) and/or rebuilding " > + "libvirt with --with-firewalld-zone")); I know you wanted this to be plain code movement, but since you're touching this error message you can apply our coding style rule [1] and unbreak the message onto one, long line. All error messages are exempt from the 80 chars rule, because it's easier to git grep them. And on a similar note, per out deprecation policy [2], firewalld-0.6.0 or older is ancient history, thus this version check can be dropped (in a separate commit of course).

> + return -1; > + } > + } > + > + return 0; > +} > + > + 1: https://libvirt.org/coding-style.html#error-message-format 2: https://libvirt.org/platforms.html#linux-freebsd-and-macos Michal