Re: [PATCH v5 12/30] network: support setting firewallBackend from network.conf

On 5/20/24 6:14 AM, Daniel P. Berrangé wrote: > On Fri, May 17, 2024 at 01:29:49PM -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 FIREWALL_BACKEND_DEFAULT_1 is available and, if so, set > > that. (Since FIREWALL_BACKEND_DEFAULT_1 is currently "iptables", this > > means checking to see it the iptables binary is present on the > > system). If the default backend isn't available, 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_default_1". This way the conf file will have correct > > information no matter what ordering is chosen for default backend at > > build time (as more backends are added, settings will be added for > > "firewall_backend_default_n", and those will be settable in > > meson_options.txt and on the meson commandline to change the ordering > > of the auto-detection when no backend is set in network.conf). > > > > virNetworkLoadDriverConfig() may look more complicated than necessary, > > but as additional backends are added, it will be easier to add checks > > for those backends (and to re-order the checks based on builders' > > preferences). > > > > Signed-off-by: Laine Stump <laine(a)redhat.com&gt; > > > @@ -62,18 +62,75 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED, > > const char *filename) > > { > > g_autoptr(virConf) conf = NULL; > > + g_autofree char *fwBackendStr = NULL; > > + bool fwBackendSelected = false; > > + size_t i; > > + int fwBackends[] = { FIREWALL_BACKEND_DEFAULT_1 }; > > + G_STATIC_ASSERT(G_N_ELEMENTS(fwBackends) == VIR_FIREWALL_BACKEND_LAST); > > I'd say we shoule remove this assert and use: > > size_t nfwBackends = G_N_ELEMENTS(fwBackends)); The reason I put in the assert was to make sure there is a compile-time error if someone adds a new backend and forgets to add an entry in the above array, or to add a new FIREWALL_BACKEND_n in the meson_options + an additional item for it in fwBackends (similar to typecasting ints into enums so the compile will catch it when you add an enum value and forget to add a case to a switch statement).

> > + if (access(filename, R_OK) == 0) { > > + > > + conf = virConfReadFile(filename, 0); > > + if (!conf) > > + return -1; > > + > > + /* use virConfGetValue*(conf, ...) functions to read any settings into cfg */ > > + > > + if (virConfGetValueString(conf, "firewall_backend", &fwBackendStr) < 0) > > + return -1; > > + > > + if (fwBackendStr) { > > + fwBackends[0] = virFirewallBackendTypeFromString(fwBackendStr); > > + > > + if (fwBackends[0] < 0) { > > + virReportError(VIR_ERR_INTERNAL_ERROR, > > + _("unrecognized 'firewall_backend = '%1$s' set in network driver config file %2$s"), > > + fwBackendStr, filename); > > + return -1; > > + } > > + VIR_INFO("firewall_backend setting requested from config file %s: '%s'", > > + virFirewallBackendTypeToString(fwBackends[0]), filename); > > Here set > > nfwBackends = 1; > > since when a config param is set, we don't want to ever fallback to > the 2nd entry That is taken care of by [see [*] below], but I'll add this suggestion anyway to reduce the brain cells required for someone reading through the code :-) (and also as a backup in case the code at [*] gets changed in the future).

> > > + } > > + } > > - /* if file doesn't exist or is unreadable, ignore the "error" */ > > - if (access(filename, R_OK) == -1) > > - return 0; > > + for (i = 0; i < G_N_ELEMENTS(fwBackends) && !fwBackendSelected; i++) { > > s/G_N_ELEMENTS(...)/nfwBackends/; > > > - conf = virConfReadFile(filename, 0); > > - if (!conf) > > - return -1; > > + switch ((virFirewallBackend)fwBackends[i]) { > > + case VIR_FIREWALL_BACKEND_IPTABLES: { > > + g_autofree char *iptablesInPath = virFindFileInPath(IPTABLES); > > - /* use virConfGetValue*(conf, ...) functions to read any settings into cfg */ > > + if (iptablesInPath) > > + fwBackendSelected = true; > > + break; > > + } > > + case VIR_FIREWALL_BACKEND_LAST: > > + virReportEnumRangeError(virFirewallBackend, fwBackends[i]); > > + return -1; > > + } > > - return 0; > > + if (fwBackendSelected) { > > + > > + cfg->firewallBackend = fwBackends[i]; > > + [*] The else clause just below this comment makes sure that if a backend is specified in the config (and thus fwBackendStr is non-null) then we'll never fall back to one of the alternates, but will instead immediately log an error and fail. And it has the added advantage (over just setting nFwBackends = 1) of giving us an easy way to differentiate the log message of "couldn't find a working backend among all the possibilities" and "the backend you specifically requested is unavailable". > > + } else if (fwBackendStr) { > > + > > + /* explicitly requested backend not found - this is a failure */ > > + virReportError(VIR_ERR_INTERNAL_ERROR, > > + _("requested firewall_backend '%1$s' is not available"), > > + virFirewallBackendTypeToString(fwBackends[i])); > > + return -1; > > + } > > + } > > + > > + if (fwBackendSelected) { > > + VIR_INFO("using firewall_backend: '%s'", > > + virFirewallBackendTypeToString(cfg->firewallBackend)); > > + return 0; > > + } else { > > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > > + _("could not find a usable firewall backend")); > > + return -1; > > + } > > } > > With regards, > Daniel