11:14 a.m.
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>
> @@ -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