On Mon, May 20, 2024 at 12:14:26PM -0400, Laine Stump wrote:
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@redhat.com>
@@ -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).
Ah right, yes.
+ 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).
Yeah, the current code works, but is a bit confusing to me. Meanwhile Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
+ } + } - /* 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
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 :|