On Fri, Jun 14, 2024 at 03:43:53PM GMT, Daniel P. Berrangé wrote:
meson.build | 26 +++++++++++++++++++------- meson_options.txt | 2 +- src/network/bridge_driver_conf.c | 19 ++++++++++++++----- src/network/bridge_driver_linux.c | 10 ++++++++++ src/network/bridge_driver_nop.c | 15 ++++++++++++++- src/util/virfirewall.c | 6 ++++++ src/util/virfirewall.h | 1 + 7 files changed, 65 insertions(+), 14 deletions(-)
The test suite no longer passes after applying this. At the very least, you need to squash in the diff at the bottom of this message.
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) - error('invalid value for firewall_backend_priority option') + if firewall_backend_priority.length() == 0 + if host_machine.system() == 'linux' + firewall_backend_priority = ['nftables', 'iptables'] + else + # No firewall impl on non-Linux so far, so force 'none' + # as placeholder + firewall_backend_priority = ['none'] + endif + else + if host_machine.system() != 'linux' + error('firewall backend priority only supported on linux hosts') + endif endif
This implementation allows things such as -Dfirewall_backend_priority=nftables and -Dfirewall_backend_priority=iptables,iptables At least -Dfirewall_backend_priority=iptables,nftables,iptables will be blocked, but only because it results in a compilation error: meson will happily accept it. Are we okay with that? It's IMO inferior to the much stricter checking that's performed today.
@@ -815,6 +816,11 @@ virFirewallApplyCmd(virFirewall *firewall, switch (virFirewallGetBackend(firewall)) { + case VIR_FIREWALL_BACKEND_NONE: + virReportError(VIR_ERR_NO_SUPPORT, + _("Firewall backend is not implemented")); + return -1;
This check (and the other ones you've introduced) are running too late, which leads to this behavior: $ cat default-session.xml <network> <name>default-session</name> <forward mode='nat'/> <bridge name='virbr0' stp='on' delay='0'/> <ip address='192.168.123.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.123.2' end='192.168.123.254'/> </dhcp> </ip> </network> $ virsh -c qemu:///session net-define default-session.xml Network default-session defined from default-session.xml $ virsh -c qemu:///session net-start default-session error: Failed to start network default-session error: error creating bridge interface virbr0: Operation not permitted Since <forward mode='nat'/> requires firewall rules, we shouldn't allow a network that uses it to be defined in the first place. diff --git a/po/POTFILES b/po/POTFILES index 4bfbb91164..1ed4086d2c 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -143,6 +143,7 @@ src/lxc/lxc_process.c src/network/bridge_driver.c src/network/bridge_driver_conf.c src/network/bridge_driver_linux.c +src/network/bridge_driver_nop.c src/network/leaseshelper.c src/network/network_iptables.c src/network/network_nftables.c diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c index 2114d521d1..323990cf17 100644 --- a/src/network/bridge_driver_nop.c +++ b/src/network/bridge_driver_nop.c @@ -49,8 +49,8 @@ int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED, */ if (firewallBackend != VIR_FIREWALL_BACKEND_NONE) { virReportError(VIR_ERR_NO_SUPPORT, - _("Firewall backend '%s' not available on this platform"), - virFirewallBackendTypeToString(firewallBackend)); + _("Firewall backend '%1$s' not available on this platform"), + virFirewallBackendTypeToSTring(firewallBackend)); return -1; } return 0; diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index d374f54b64..090dbcdbed 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -817,7 +817,7 @@ virFirewallApplyCmd(virFirewall *firewall, switch (virFirewallGetBackend(firewall)) { case VIR_FIREWALL_BACKEND_NONE: - virReportError(VIR_ERR_NO_SUPPORT, + virReportError(VIR_ERR_NO_SUPPORT, "%s", _("Firewall backend is not implemented")); return -1; -- Andrea Bolognani / Red Hat / Virtualization