11:50 a.m.
On 5/28/24 12:31 PM, Pavel Hrdina wrote:
On Tue, May 28, 2024 at 05:49:19PM +0200, Andrea Bolognani wrote:
> The current implementation requires users to configure the
> preference as such:
>
> -Dfirewall_backend_default_1=iptables
> -Dfirewall_backend_default_2=nftables
>
> In addition to being more verbose than one would hope, there
> are several things that could go wrong.
>
> First of all, meson performs no validation on the provided
> values, so mistakes will only be caught by the compiler.
> Additionally, it's entirely possible to provide nonsensical
> combinations, such as repeating the same value twice.
>
> Change things so that the preference can now be configured
> as such:
>
> -Dfirewall_backend_priority=iptables,nftables
>
> Checks have been added to prevent invalid values from being
> accepted.
>
> Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
> ---
> meson.build | 16 +++++++++-------
> meson_options.txt | 3 +--
> src/network/bridge_driver_conf.c | 6 +++++-
> src/network/meson.build | 6 ++++--
> src/network/network.conf.in | 13 +++++++------
> 5 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index f67c3d2724..ed0e9686f8 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1635,15 +1635,17 @@ endif
>
> if not get_option('driver_network').disabled() and
conf.has('WITH_LIBVIRTD')
> conf.set('WITH_NETWORK', 1)
> - firewall_backend_default_1 = get_option('firewall_backend_default_1')
> - firewall_backend_default_conf = firewall_backend_default_1
> - firewall_backend_default_1 = 'VIR_FIREWALL_BACKEND_' +
firewall_backend_default_1.to_upper()
> - conf.set('FIREWALL_BACKEND_DEFAULT_1', firewall_backend_default_1)
>
> - firewall_backend_default_2 = get_option('firewall_backend_default_2')
> - firewall_backend_default_2 = 'VIR_FIREWALL_BACKEND_' +
firewall_backend_default_2.to_upper()
> - conf.set('FIREWALL_BACKEND_DEFAULT_2', firewall_backend_default_2)
> + firewall_backend_priority = get_option('firewall_backend_priority')
> + if (not firewall_backend_priority.contains('nftables') or
> + not firewall_backend_priority.contains('iptables') or
> + firewall_backend_priority.length() != 2)
No need to have a check for specific values. Meson will already check if
they are from the list of choices defined in meson_options.txt .
But we don't just need to check that the values in the list are all
valid options; we also want to make sure that nobody has entered the
same value multiple times (which this ends up doing by making sure that
there is at least one entry for each valid value, and that the list is
the same length as the number of valid values).
Or do we not care if someone repeats the same value? Maybe somebody
wants to include iptables support in the build, but not look for it
automatically (instead only use it if it's explicitly requested in
network.conf). One way of doing that would be to sent
firewall_backend_priority = nftables,nftables
(that does seem a bit obtuse; perhaps it would be better to allow
limiting the length of the option list to 1)
Anyway, I'm fine with it either way. Both are better than what I had before.
> + error('invalid value for firewall_backend_priority option')
> + endif
>
> + conf.set('FIREWALL_BACKEND_PRIORITY_0', 'VIR_FIREWALL_BACKEND_' +
firewall_backend_priority[0].to_upper())
> + conf.set('FIREWALL_BACKEND_PRIORITY_1', 'VIR_FIREWALL_BACKEND_' +
firewall_backend_priority[1].to_upper())
> + conf.set('FIREWALL_BACKEND_PRIORITY_NUM',
firewall_backend_priority.length())
> elif get_option('driver_network').enabled()
> error('libvirtd must be enabled to build the network driver')
> endif
> diff --git a/meson_options.txt b/meson_options.txt
> index ad354a8668..8723d13231 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -115,8 +115,7 @@ option('dtrace', type: 'feature', value:
'auto', description: 'use dtrace for st
> option('firewalld', type: 'feature', value: 'auto',
description: 'firewalld support')
> # dep:firewalld
> option('firewalld_zone', type: 'feature', value: 'auto',
description: 'whether to install firewalld libvirt zone')
> -option('firewall_backend_default_1', type: 'string', value:
'nftables', description: 'first firewall backend to try when none is
specified')
> -option('firewall_backend_default_2', type: 'string', value:
'iptables', description: 'second firewall backend to try when none is
specified (and first is unavailable)')
> +option('firewall_backend_priority', type: 'array', choices:
['nftables', 'iptables'], description: 'firewall backends to try,
preferred ones first')
What about "order of firewall backends to try"? The part "preferred ones
first" sounds strange to me.
Otherwise looks good.
Pavel