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@xxxxxxxxxx>
> ---
> 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 .
> + 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
Attachment:
signature.asc
Description: PGP signature
