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@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).
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@xxxxxxxxxx>
>
> >
> > > + }
> > > + }
> > > - /* 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 :|
