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@xxxxxxxxxx>
@@ -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
