[libvirt PATCH 00/12] Clean up cruft in firewall/iptables code (in preparation for nftables)
These patches make no functional change, they just remove a bunch of cruft that accumulated over the years and is no longer needed. This is all in advance of adding support for native nftable support, but there is nothing nftables-specific being added here; I just wanted to get these cleanups out of way now so that the eventual nftables support patchset is smaller and less complicated. (NB: the concept of a "firewall backend" is being removed here, implying that it will no longer exist. This is not true, but the way that it will exist in the future will be different (per-firewall object rather than per-process) so almost all of the existing code won't be applicable anyway.) Laine Stump (12): network: eliminate code that uses default iptables chains util: rename/move iptablesFormatNetwork to virSocketAddrFormatWithPrefix util: rename iptables operators to something less generic tests: remove firewalld backend tests from virfirewalltest.c tests: remove unnecessary ret variables and cleanup labels tests: document why virgdbus must be mocked in networkxml2firewalltest.c util: eliminate pointless switch in virFirewallApplyRule util: simplify virFirewallBackendSynchronize() util: move and rename virFirewallBackendSynchronize() util: remove check for iptables binary during virFirewallInit util: remove currentBackend from virfirewall.c util: remove virFirewallOnceInit() src/libvirt_private.syms | 5 +- src/network/bridge_driver_linux.c | 37 +-- src/util/virfirewall.c | 143 +---------- src/util/virfirewall.h | 2 - src/util/virfirewalld.c | 43 ++++ src/util/virfirewalld.h | 2 + src/util/virfirewallpriv.h | 37 --- src/util/viriptables.c | 207 +++++++--------- src/util/viriptables.h | 2 - src/util/virsocketaddr.c | 44 ++++ src/util/virsocketaddr.h | 3 + tests/networkxml2firewalltest.c | 14 +- tests/nwfilterebiptablestest.c | 7 - tests/nwfilterxml2firewalltest.c | 8 +- tests/virfirewalltest.c | 390 ++++-------------------------- 15 files changed, 247 insertions(+), 697 deletions(-) delete mode 100644 src/util/virfirewallpriv.h -- 2.33.1
The network driver has put all its rules into private chains (created by libvirt) since commit 7431b3eb9a, which was included in libvirt-5.1.0. When the conversion was made, code was included that would attempt to delete existing rules in the default chains, to make it possible to upgrade libvirt without restarting the host OS. Almost 3 years has passed, and it is doubtful that anyone will be attempting to upgrade directly from a pre-5.1.0 libvirt to something as new as 8.0.0 (possibly with the exception of upgrading the entire OS to a new release, which would include also rebooting), so it is now safe to remove this code. Signed-off-by: Laine Stump <laine@redhat.com> --- src/libvirt_private.syms | 1 - src/network/bridge_driver_linux.c | 37 ++--------- src/util/viriptables.c | 104 ++++++++++++------------------ src/util/viriptables.h | 2 - 4 files changed, 49 insertions(+), 95 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7be5b51100..ff6f71054e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2501,7 +2501,6 @@ iptablesRemoveTcpInput; iptablesRemoveTcpOutput; iptablesRemoveUdpInput; iptablesRemoveUdpOutput; -iptablesSetDeletePrivate; iptablesSetupPrivateChains; diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index d2eab33e5f..1c8be7103a 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -37,7 +37,7 @@ VIR_LOG_INIT("network.bridge_driver_linux"); static virOnceControl createdOnce; static bool chainInitDone; /* true iff networkSetupPrivateChains was ever called */ -static bool createdChains; /* true iff networkSetupPrivateChains created chains during most recent call */ + static virErrorPtr errInitV4; static virErrorPtr errInitV6; @@ -50,7 +50,6 @@ static void networkSetupPrivateChains(void) VIR_DEBUG("Setting up global firewall chains"); - createdChains = false; virFreeError(errInitV4); errInitV4 = NULL; virFreeError(errInitV6); @@ -63,12 +62,10 @@ static void networkSetupPrivateChains(void) errInitV4 = virSaveLastError(); virResetLastError(); } else { - if (rc) { + if (rc) VIR_DEBUG("Created global IPv4 chains"); - createdChains = true; - } else { + else VIR_DEBUG("Global IPv4 chains already exist"); - } } rc = iptablesSetupPrivateChains(VIR_FIREWALL_LAYER_IPV6); @@ -78,12 +75,10 @@ static void networkSetupPrivateChains(void) errInitV6 = virSaveLastError(); virResetLastError(); } else { - if (rc) { + if (rc) VIR_DEBUG("Created global IPv6 chains"); - createdChains = true; - } else { + else VIR_DEBUG("Global IPv6 chains already exist"); - } } chainInitDone = true; @@ -145,7 +140,7 @@ networkHasRunningNetworksWithFW(virNetworkDriverState *driver) void networkPreReloadFirewallRules(virNetworkDriverState *driver, - bool startup, + bool startup G_GNUC_UNUSED, bool force) { /* @@ -183,31 +178,13 @@ networkPreReloadFirewallRules(virNetworkDriverState *driver, } ignore_value(virOnce(&createdOnce, networkSetupPrivateChains)); - - /* - * If this is initial startup, and we just created the - * top level private chains we either - * - * - upgraded from old libvirt - * - freshly booted from clean state - * - * In the first case we must delete the old rules from - * the built-in chains, instead of our new private chains. - * In the second case it doesn't matter, since no existing - * rules will be present. Thus we can safely just tell it - * to always delete from the builin chain - */ - if (startup && createdChains) { - VIR_DEBUG("Requesting cleanup of legacy firewall rules"); - iptablesSetDeletePrivate(false); - } } } void networkPostReloadFirewallRules(bool startup G_GNUC_UNUSED) { - iptablesSetDeletePrivate(true); + } diff --git a/src/util/viriptables.c b/src/util/viriptables.c index 721e1eeae7..ac949efba7 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -47,8 +47,6 @@ enum { REMOVE }; -static bool deletePrivate = true; - typedef struct { const char *parent; const char *child; @@ -162,17 +160,9 @@ iptablesSetupPrivateChains(virFirewallLayer layer) } -void -iptablesSetDeletePrivate(bool pvt) -{ - deletePrivate = pvt; -} - - static void iptablesInput(virFirewall *fw, virFirewallLayer layer, - bool pvt, const char *iface, int port, int action, @@ -186,7 +176,7 @@ iptablesInput(virFirewall *fw, virFirewallAddRule(fw, layer, "--table", "filter", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_INP" : "INPUT", + "LIBVIRT_INP", "--in-interface", iface, "--protocol", tcp ? "tcp" : "udp", "--destination-port", portstr, @@ -197,7 +187,6 @@ iptablesInput(virFirewall *fw, static void iptablesOutput(virFirewall *fw, virFirewallLayer layer, - bool pvt, const char *iface, int port, int action, @@ -211,7 +200,7 @@ iptablesOutput(virFirewall *fw, virFirewallAddRule(fw, layer, "--table", "filter", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_OUT" : "OUTPUT", + "LIBVIRT_OUT", "--out-interface", iface, "--protocol", tcp ? "tcp" : "udp", "--destination-port", portstr, @@ -234,7 +223,7 @@ iptablesAddTcpInput(virFirewall *fw, const char *iface, int port) { - iptablesInput(fw, layer, true, iface, port, ADD, 1); + iptablesInput(fw, layer, iface, port, ADD, 1); } /** @@ -252,7 +241,7 @@ iptablesRemoveTcpInput(virFirewall *fw, const char *iface, int port) { - iptablesInput(fw, layer, deletePrivate, iface, port, REMOVE, 1); + iptablesInput(fw, layer, iface, port, REMOVE, 1); } /** @@ -270,7 +259,7 @@ iptablesAddUdpInput(virFirewall *fw, const char *iface, int port) { - iptablesInput(fw, layer, true, iface, port, ADD, 0); + iptablesInput(fw, layer, iface, port, ADD, 0); } /** @@ -288,7 +277,7 @@ iptablesRemoveUdpInput(virFirewall *fw, const char *iface, int port) { - iptablesInput(fw, layer, deletePrivate, iface, port, REMOVE, 0); + iptablesInput(fw, layer, iface, port, REMOVE, 0); } /** @@ -306,7 +295,7 @@ iptablesAddTcpOutput(virFirewall *fw, const char *iface, int port) { - iptablesOutput(fw, layer, true, iface, port, ADD, 1); + iptablesOutput(fw, layer, iface, port, ADD, 1); } /** @@ -324,7 +313,7 @@ iptablesRemoveTcpOutput(virFirewall *fw, const char *iface, int port) { - iptablesOutput(fw, layer, deletePrivate, iface, port, REMOVE, 1); + iptablesOutput(fw, layer, iface, port, REMOVE, 1); } /** @@ -342,7 +331,7 @@ iptablesAddUdpOutput(virFirewall *fw, const char *iface, int port) { - iptablesOutput(fw, layer, true, iface, port, ADD, 0); + iptablesOutput(fw, layer, iface, port, ADD, 0); } /** @@ -360,7 +349,7 @@ iptablesRemoveUdpOutput(virFirewall *fw, const char *iface, int port) { - iptablesOutput(fw, layer, deletePrivate, iface, port, REMOVE, 0); + iptablesOutput(fw, layer, iface, port, REMOVE, 0); } @@ -400,7 +389,6 @@ static char *iptablesFormatNetwork(virSocketAddr *netaddr, */ static int iptablesForwardAllowOut(virFirewall *fw, - bool pvt, virSocketAddr *netaddr, unsigned int prefix, const char *iface, @@ -418,7 +406,7 @@ iptablesForwardAllowOut(virFirewall *fw, virFirewallAddRule(fw, layer, "--table", "filter", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_FWO" : "FORWARD", + "LIBVIRT_FWO", "--source", networkstr, "--in-interface", iface, "--out-interface", physdev, @@ -428,7 +416,7 @@ iptablesForwardAllowOut(virFirewall *fw, virFirewallAddRule(fw, layer, "--table", "filter", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_FWO" : "FORWARD", + "LIBVIRT_FWO", "--source", networkstr, "--in-interface", iface, "--jump", "ACCEPT", @@ -457,7 +445,7 @@ iptablesAddForwardAllowOut(virFirewall *fw, const char *iface, const char *physdev) { - return iptablesForwardAllowOut(fw, true, netaddr, prefix, iface, physdev, ADD); + return iptablesForwardAllowOut(fw, netaddr, prefix, iface, physdev, ADD); } /** @@ -480,7 +468,7 @@ iptablesRemoveForwardAllowOut(virFirewall *fw, const char *iface, const char *physdev) { - return iptablesForwardAllowOut(fw, deletePrivate, netaddr, prefix, iface, physdev, REMOVE); + return iptablesForwardAllowOut(fw, netaddr, prefix, iface, physdev, REMOVE); } @@ -489,7 +477,6 @@ iptablesRemoveForwardAllowOut(virFirewall *fw, */ static int iptablesForwardAllowRelatedIn(virFirewall *fw, - bool pvt, virSocketAddr *netaddr, unsigned int prefix, const char *iface, @@ -507,7 +494,7 @@ iptablesForwardAllowRelatedIn(virFirewall *fw, virFirewallAddRule(fw, layer, "--table", "filter", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_FWI" : "FORWARD", + "LIBVIRT_FWI", "--destination", networkstr, "--in-interface", physdev, "--out-interface", iface, @@ -519,7 +506,7 @@ iptablesForwardAllowRelatedIn(virFirewall *fw, virFirewallAddRule(fw, layer, "--table", "filter", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_FWI" : "FORWARD", + "LIBVIRT_FWI", "--destination", networkstr, "--out-interface", iface, "--match", "conntrack", @@ -550,7 +537,7 @@ iptablesAddForwardAllowRelatedIn(virFirewall *fw, const char *iface, const char *physdev) { - return iptablesForwardAllowRelatedIn(fw, true, netaddr, prefix, iface, physdev, ADD); + return iptablesForwardAllowRelatedIn(fw, netaddr, prefix, iface, physdev, ADD); } /** @@ -573,14 +560,13 @@ iptablesRemoveForwardAllowRelatedIn(virFirewall *fw, const char *iface, const char *physdev) { - return iptablesForwardAllowRelatedIn(fw, deletePrivate, netaddr, prefix, iface, physdev, REMOVE); + return iptablesForwardAllowRelatedIn(fw, netaddr, prefix, iface, physdev, REMOVE); } /* Allow all traffic destined to the bridge, with a valid network address */ static int iptablesForwardAllowIn(virFirewall *fw, - bool pvt, virSocketAddr *netaddr, unsigned int prefix, const char *iface, @@ -598,7 +584,7 @@ iptablesForwardAllowIn(virFirewall *fw, virFirewallAddRule(fw, layer, "--table", "filter", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_FWI" : "FORWARD", + "LIBVIRT_FWI", "--destination", networkstr, "--in-interface", physdev, "--out-interface", iface, @@ -608,7 +594,7 @@ iptablesForwardAllowIn(virFirewall *fw, virFirewallAddRule(fw, layer, "--table", "filter", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_FWI" : "FORWARD", + "LIBVIRT_FWI", "--destination", networkstr, "--out-interface", iface, "--jump", "ACCEPT", @@ -636,7 +622,7 @@ iptablesAddForwardAllowIn(virFirewall *fw, const char *iface, const char *physdev) { - return iptablesForwardAllowIn(fw, true, netaddr, prefix, iface, physdev, ADD); + return iptablesForwardAllowIn(fw, netaddr, prefix, iface, physdev, ADD); } /** @@ -659,20 +645,19 @@ iptablesRemoveForwardAllowIn(virFirewall *fw, const char *iface, const char *physdev) { - return iptablesForwardAllowIn(fw, deletePrivate, netaddr, prefix, iface, physdev, REMOVE); + return iptablesForwardAllowIn(fw, netaddr, prefix, iface, physdev, REMOVE); } static void iptablesForwardAllowCross(virFirewall *fw, virFirewallLayer layer, - bool pvt, const char *iface, int action) { virFirewallAddRule(fw, layer, "--table", "filter", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_FWX" : "FORWARD", + "LIBVIRT_FWX", "--in-interface", iface, "--out-interface", iface, "--jump", "ACCEPT", @@ -695,7 +680,7 @@ iptablesAddForwardAllowCross(virFirewall *fw, virFirewallLayer layer, const char *iface) { - iptablesForwardAllowCross(fw, layer, true, iface, ADD); + iptablesForwardAllowCross(fw, layer, iface, ADD); } /** @@ -714,20 +699,19 @@ iptablesRemoveForwardAllowCross(virFirewall *fw, virFirewallLayer layer, const char *iface) { - iptablesForwardAllowCross(fw, layer, deletePrivate, iface, REMOVE); + iptablesForwardAllowCross(fw, layer, iface, REMOVE); } static void iptablesForwardRejectOut(virFirewall *fw, virFirewallLayer layer, - bool pvt, const char *iface, int action) { virFirewallAddRule(fw, layer, "--table", "filter", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_FWO" : "FORWARD", + "LIBVIRT_FWO", "--in-interface", iface, "--jump", "REJECT", NULL); @@ -748,7 +732,7 @@ iptablesAddForwardRejectOut(virFirewall *fw, virFirewallLayer layer, const char *iface) { - iptablesForwardRejectOut(fw, layer, true, iface, ADD); + iptablesForwardRejectOut(fw, layer, iface, ADD); } /** @@ -766,21 +750,20 @@ iptablesRemoveForwardRejectOut(virFirewall *fw, virFirewallLayer layer, const char *iface) { - iptablesForwardRejectOut(fw, layer, deletePrivate, iface, REMOVE); + iptablesForwardRejectOut(fw, layer, iface, REMOVE); } static void iptablesForwardRejectIn(virFirewall *fw, virFirewallLayer layer, - bool pvt, const char *iface, int action) { virFirewallAddRule(fw, layer, "--table", "filter", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_FWI" : "FORWARD", + "LIBVIRT_FWI", "--out-interface", iface, "--jump", "REJECT", NULL); @@ -801,7 +784,7 @@ iptablesAddForwardRejectIn(virFirewall *fw, virFirewallLayer layer, const char *iface) { - iptablesForwardRejectIn(fw, layer, true, iface, ADD); + iptablesForwardRejectIn(fw, layer, iface, ADD); } /** @@ -819,7 +802,7 @@ iptablesRemoveForwardRejectIn(virFirewall *fw, virFirewallLayer layer, const char *iface) { - iptablesForwardRejectIn(fw, layer, deletePrivate, iface, REMOVE); + iptablesForwardRejectIn(fw, layer, iface, REMOVE); } @@ -828,7 +811,6 @@ iptablesRemoveForwardRejectIn(virFirewall *fw, */ static int iptablesForwardMasquerade(virFirewall *fw, - bool pvt, virSocketAddr *netaddr, unsigned int prefix, const char *physdev, @@ -863,7 +845,7 @@ iptablesForwardMasquerade(virFirewall *fw, rule = virFirewallAddRule(fw, layer, "--table", "nat", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_PRT" : "POSTROUTING", + "LIBVIRT_PRT", "--source", networkstr, "-p", protocol, "!", "--destination", networkstr, @@ -872,7 +854,7 @@ iptablesForwardMasquerade(virFirewall *fw, rule = virFirewallAddRule(fw, layer, "--table", "nat", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_PRT" : "POSTROUTING", + "LIBVIRT_PRT", "--source", networkstr, "!", "--destination", networkstr, NULL); @@ -944,7 +926,7 @@ iptablesAddForwardMasquerade(virFirewall *fw, virPortRange *port, const char *protocol) { - return iptablesForwardMasquerade(fw, true, netaddr, prefix, + return iptablesForwardMasquerade(fw, netaddr, prefix, physdev, addr, port, protocol, ADD); } @@ -970,7 +952,7 @@ iptablesRemoveForwardMasquerade(virFirewall *fw, virPortRange *port, const char *protocol) { - return iptablesForwardMasquerade(fw, deletePrivate, netaddr, prefix, + return iptablesForwardMasquerade(fw, netaddr, prefix, physdev, addr, port, protocol, REMOVE); } @@ -980,7 +962,6 @@ iptablesRemoveForwardMasquerade(virFirewall *fw, */ static int iptablesForwardDontMasquerade(virFirewall *fw, - bool pvt, virSocketAddr *netaddr, unsigned int prefix, const char *physdev, @@ -998,7 +979,7 @@ iptablesForwardDontMasquerade(virFirewall *fw, virFirewallAddRule(fw, layer, "--table", "nat", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_PRT" : "POSTROUTING", + "LIBVIRT_PRT", "--out-interface", physdev, "--source", networkstr, "--destination", destaddr, @@ -1008,7 +989,7 @@ iptablesForwardDontMasquerade(virFirewall *fw, virFirewallAddRule(fw, layer, "--table", "nat", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_PRT" : "POSTROUTING", + "LIBVIRT_PRT", "--source", networkstr, "--destination", destaddr, "--jump", "RETURN", @@ -1038,7 +1019,7 @@ iptablesAddDontMasquerade(virFirewall *fw, const char *physdev, const char *destaddr) { - return iptablesForwardDontMasquerade(fw, true, netaddr, prefix, + return iptablesForwardDontMasquerade(fw, netaddr, prefix, physdev, destaddr, ADD); } @@ -1063,14 +1044,13 @@ iptablesRemoveDontMasquerade(virFirewall *fw, const char *physdev, const char *destaddr) { - return iptablesForwardDontMasquerade(fw, deletePrivate, netaddr, prefix, + return iptablesForwardDontMasquerade(fw, netaddr, prefix, physdev, destaddr, REMOVE); } static void iptablesOutputFixUdpChecksum(virFirewall *fw, - bool pvt, const char *iface, int port, int action) @@ -1083,7 +1063,7 @@ iptablesOutputFixUdpChecksum(virFirewall *fw, virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4, "--table", "mangle", action == ADD ? "--insert" : "--delete", - pvt ? "LIBVIRT_PRT" : "POSTROUTING", + "LIBVIRT_PRT", "--out-interface", iface, "--protocol", "udp", "--destination-port", portstr, @@ -1107,7 +1087,7 @@ iptablesAddOutputFixUdpChecksum(virFirewall *fw, const char *iface, int port) { - iptablesOutputFixUdpChecksum(fw, true, iface, port, ADD); + iptablesOutputFixUdpChecksum(fw, iface, port, ADD); } /** @@ -1124,5 +1104,5 @@ iptablesRemoveOutputFixUdpChecksum(virFirewall *fw, const char *iface, int port) { - iptablesOutputFixUdpChecksum(fw, deletePrivate, iface, port, REMOVE); + iptablesOutputFixUdpChecksum(fw, iface, port, REMOVE); } diff --git a/src/util/viriptables.h b/src/util/viriptables.h index 41c493d3eb..bb13f3292d 100644 --- a/src/util/viriptables.h +++ b/src/util/viriptables.h @@ -25,8 +25,6 @@ int iptablesSetupPrivateChains (virFirewallLayer layer); -void iptablesSetDeletePrivate (bool pvt); - void iptablesAddTcpInput (virFirewall *fw, virFirewallLayer layer, const char *iface, -- 2.33.1
This function formats an address + prefix as, e.g. 192.168.122.0/24, which is useful in places other than iptables. Move it to virsocketaddr.c and make it public so that others can use it. While moving, the bit that masks off the host bits of the address is made optional, so that the function is more generally useful. Signed-off-by: Laine Stump <laine@redhat.com> --- src/libvirt_private.syms | 1 + src/util/viriptables.c | 41 +++++-------------------------------- src/util/virsocketaddr.c | 44 ++++++++++++++++++++++++++++++++++++++++ src/util/virsocketaddr.h | 3 +++ 4 files changed, 53 insertions(+), 36 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ff6f71054e..72b38a970d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3269,6 +3269,7 @@ virSocketAddrCheckNetmask; virSocketAddrEqual; virSocketAddrFormat; virSocketAddrFormatFull; +virSocketAddrFormatWithPrefix; virSocketAddrGetIPPrefix; virSocketAddrGetNumNetmaskBits; virSocketAddrGetPath; diff --git a/src/util/viriptables.c b/src/util/viriptables.c index ac949efba7..78d979cfe8 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -353,37 +353,6 @@ iptablesRemoveUdpOutput(virFirewall *fw, } -static char *iptablesFormatNetwork(virSocketAddr *netaddr, - unsigned int prefix) -{ - virSocketAddr network; - g_autofree char *netstr = NULL; - char *ret; - - if (!(VIR_SOCKET_ADDR_IS_FAMILY(netaddr, AF_INET) || - VIR_SOCKET_ADDR_IS_FAMILY(netaddr, AF_INET6))) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only IPv4 or IPv6 addresses can be used with iptables")); - return NULL; - } - - if (virSocketAddrMaskByPrefix(netaddr, prefix, &network) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failure to mask address")); - return NULL; - } - - netstr = virSocketAddrFormat(&network); - - if (!netstr) - return NULL; - - ret = g_strdup_printf("%s/%d", netstr, prefix); - - return ret; -} - - /* Allow all traffic coming from the bridge, with a valid network address * to proceed to WAN */ @@ -399,7 +368,7 @@ iptablesForwardAllowOut(virFirewall *fw, virFirewallLayer layer = VIR_SOCKET_ADDR_FAMILY(netaddr) == AF_INET ? VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; - if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) + if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) return -1; if (physdev && physdev[0]) @@ -487,7 +456,7 @@ iptablesForwardAllowRelatedIn(virFirewall *fw, VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; g_autofree char *networkstr = NULL; - if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) + if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) return -1; if (physdev && physdev[0]) @@ -577,7 +546,7 @@ iptablesForwardAllowIn(virFirewall *fw, VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; g_autofree char *networkstr = NULL; - if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) + if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) return -1; if (physdev && physdev[0]) @@ -829,7 +798,7 @@ iptablesForwardMasquerade(virFirewall *fw, virFirewallLayer layer = af == AF_INET ? VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; - if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) + if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) return -1; if (VIR_SOCKET_ADDR_IS_FAMILY(&addr->start, af)) { @@ -972,7 +941,7 @@ iptablesForwardDontMasquerade(virFirewall *fw, virFirewallLayer layer = VIR_SOCKET_ADDR_FAMILY(netaddr) == AF_INET ? VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; - if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) + if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) return -1; if (physdev && physdev[0]) diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 94cbfc6264..430e43f2eb 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -511,6 +511,50 @@ virSocketAddrFormatFull(const virSocketAddr *addr, } +/* + * virSocketAddrFormatWithPrefix: + * @addr: an initialized virSocketAddr * + * @prefix: an IP network prefix (0-32 if IPv4, 0-128 if IPv6) + * @masked: true to mask off the host bits of the address + * + * Returns a string representation of the IP network described by + * @netaddr/@prefix. If @masked is true, the address is masked to + * remove the host bits according to prefix. So, for example, sending + * f(1.2.3.4, 24, true) would return "1.2.3.0/24", but f(1.2.3.4, 24, + * false) would return "1.2.3.4/24". + * + * returns false on failure (and logs an error message) + */ +char * +virSocketAddrFormatWithPrefix(virSocketAddr *addr, + unsigned int prefix, + bool masked) +{ + virSocketAddr network; + g_autofree char *netstr = NULL; + + if (!(VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET) || + VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET6))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Only IPv4 or IPv6 addresses can be used with a prefix")); + return NULL; + } + + if (masked && virSocketAddrMaskByPrefix(addr, prefix, &network) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failure to mask address")); + return NULL; + } + + netstr = virSocketAddrFormat(&network); + + if (!netstr) + return NULL; + + return g_strdup_printf("%s/%d", netstr, prefix); +} + + /* * virSocketAddrSetPort: * @addr: an initialized virSocketAddr * diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index f76e229730..ec265d6e44 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -88,6 +88,9 @@ char *virSocketAddrFormat(const virSocketAddr *addr); char *virSocketAddrFormatFull(const virSocketAddr *addr, bool withService, const char *separator); +char *virSocketAddrFormatWithPrefix(virSocketAddr *addr, + unsigned int prefix, + bool masked); char *virSocketAddrGetPath(virSocketAddr *addr); -- 2.33.1
Rather than calling these "ADD" and "REMOVE", which could be confused with some other random items with the same names, make them more specific by prepending "VIR_NETFILTER_" (because they will also be used by the nftables backend) and rename them to match the iptables/nftables operators they signify, i.e. INSERT and DELETE, just to eliminate confusion (in particular, in case someone ever decides that we need to also use the nftables "add" operator, which appends a rule to a chain rather than inserting it at the beginning of the chain). Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/viriptables.c | 97 +++++++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 44 deletions(-) diff --git a/src/util/viriptables.c b/src/util/viriptables.c index 78d979cfe8..d2bc10a652 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -43,8 +43,8 @@ VIR_LOG_INIT("util.iptables"); #define VIR_FROM_THIS VIR_FROM_NONE enum { - ADD = 0, - REMOVE + VIR_NETFILTER_INSERT = 0, + VIR_NETFILTER_DELETE }; typedef struct { @@ -175,7 +175,7 @@ iptablesInput(virFirewall *fw, virFirewallAddRule(fw, layer, "--table", "filter", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_INP", "--in-interface", iface, "--protocol", tcp ? "tcp" : "udp", @@ -199,7 +199,7 @@ iptablesOutput(virFirewall *fw, virFirewallAddRule(fw, layer, "--table", "filter", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_OUT", "--out-interface", iface, "--protocol", tcp ? "tcp" : "udp", @@ -223,7 +223,7 @@ iptablesAddTcpInput(virFirewall *fw, const char *iface, int port) { - iptablesInput(fw, layer, iface, port, ADD, 1); + iptablesInput(fw, layer, iface, port, VIR_NETFILTER_INSERT, 1); } /** @@ -241,7 +241,7 @@ iptablesRemoveTcpInput(virFirewall *fw, const char *iface, int port) { - iptablesInput(fw, layer, iface, port, REMOVE, 1); + iptablesInput(fw, layer, iface, port, VIR_NETFILTER_DELETE, 1); } /** @@ -259,7 +259,7 @@ iptablesAddUdpInput(virFirewall *fw, const char *iface, int port) { - iptablesInput(fw, layer, iface, port, ADD, 0); + iptablesInput(fw, layer, iface, port, VIR_NETFILTER_INSERT, 0); } /** @@ -277,7 +277,7 @@ iptablesRemoveUdpInput(virFirewall *fw, const char *iface, int port) { - iptablesInput(fw, layer, iface, port, REMOVE, 0); + iptablesInput(fw, layer, iface, port, VIR_NETFILTER_DELETE, 0); } /** @@ -295,7 +295,7 @@ iptablesAddTcpOutput(virFirewall *fw, const char *iface, int port) { - iptablesOutput(fw, layer, iface, port, ADD, 1); + iptablesOutput(fw, layer, iface, port, VIR_NETFILTER_INSERT, 1); } /** @@ -313,7 +313,7 @@ iptablesRemoveTcpOutput(virFirewall *fw, const char *iface, int port) { - iptablesOutput(fw, layer, iface, port, REMOVE, 1); + iptablesOutput(fw, layer, iface, port, VIR_NETFILTER_DELETE, 1); } /** @@ -331,7 +331,7 @@ iptablesAddUdpOutput(virFirewall *fw, const char *iface, int port) { - iptablesOutput(fw, layer, iface, port, ADD, 0); + iptablesOutput(fw, layer, iface, port, VIR_NETFILTER_INSERT, 0); } /** @@ -349,7 +349,7 @@ iptablesRemoveUdpOutput(virFirewall *fw, const char *iface, int port) { - iptablesOutput(fw, layer, iface, port, REMOVE, 0); + iptablesOutput(fw, layer, iface, port, VIR_NETFILTER_DELETE, 0); } @@ -374,7 +374,7 @@ iptablesForwardAllowOut(virFirewall *fw, if (physdev && physdev[0]) virFirewallAddRule(fw, layer, "--table", "filter", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_FWO", "--source", networkstr, "--in-interface", iface, @@ -384,7 +384,7 @@ iptablesForwardAllowOut(virFirewall *fw, else virFirewallAddRule(fw, layer, "--table", "filter", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_FWO", "--source", networkstr, "--in-interface", iface, @@ -414,7 +414,8 @@ iptablesAddForwardAllowOut(virFirewall *fw, const char *iface, const char *physdev) { - return iptablesForwardAllowOut(fw, netaddr, prefix, iface, physdev, ADD); + return iptablesForwardAllowOut(fw, netaddr, prefix, iface, physdev, + VIR_NETFILTER_INSERT); } /** @@ -437,7 +438,8 @@ iptablesRemoveForwardAllowOut(virFirewall *fw, const char *iface, const char *physdev) { - return iptablesForwardAllowOut(fw, netaddr, prefix, iface, physdev, REMOVE); + return iptablesForwardAllowOut(fw, netaddr, prefix, iface, physdev, + VIR_NETFILTER_DELETE); } @@ -462,7 +464,7 @@ iptablesForwardAllowRelatedIn(virFirewall *fw, if (physdev && physdev[0]) virFirewallAddRule(fw, layer, "--table", "filter", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_FWI", "--destination", networkstr, "--in-interface", physdev, @@ -474,7 +476,7 @@ iptablesForwardAllowRelatedIn(virFirewall *fw, else virFirewallAddRule(fw, layer, "--table", "filter", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_FWI", "--destination", networkstr, "--out-interface", iface, @@ -506,7 +508,8 @@ iptablesAddForwardAllowRelatedIn(virFirewall *fw, const char *iface, const char *physdev) { - return iptablesForwardAllowRelatedIn(fw, netaddr, prefix, iface, physdev, ADD); + return iptablesForwardAllowRelatedIn(fw, netaddr, prefix, iface, physdev, + VIR_NETFILTER_INSERT); } /** @@ -529,7 +532,8 @@ iptablesRemoveForwardAllowRelatedIn(virFirewall *fw, const char *iface, const char *physdev) { - return iptablesForwardAllowRelatedIn(fw, netaddr, prefix, iface, physdev, REMOVE); + return iptablesForwardAllowRelatedIn(fw, netaddr, prefix, iface, physdev, + VIR_NETFILTER_DELETE); } /* Allow all traffic destined to the bridge, with a valid network address @@ -552,7 +556,7 @@ iptablesForwardAllowIn(virFirewall *fw, if (physdev && physdev[0]) virFirewallAddRule(fw, layer, "--table", "filter", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_FWI", "--destination", networkstr, "--in-interface", physdev, @@ -562,7 +566,7 @@ iptablesForwardAllowIn(virFirewall *fw, else virFirewallAddRule(fw, layer, "--table", "filter", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_FWI", "--destination", networkstr, "--out-interface", iface, @@ -591,7 +595,8 @@ iptablesAddForwardAllowIn(virFirewall *fw, const char *iface, const char *physdev) { - return iptablesForwardAllowIn(fw, netaddr, prefix, iface, physdev, ADD); + return iptablesForwardAllowIn(fw, netaddr, prefix, iface, physdev, + VIR_NETFILTER_INSERT); } /** @@ -614,7 +619,8 @@ iptablesRemoveForwardAllowIn(virFirewall *fw, const char *iface, const char *physdev) { - return iptablesForwardAllowIn(fw, netaddr, prefix, iface, physdev, REMOVE); + return iptablesForwardAllowIn(fw, netaddr, prefix, iface, physdev, + VIR_NETFILTER_DELETE); } static void @@ -625,7 +631,7 @@ iptablesForwardAllowCross(virFirewall *fw, { virFirewallAddRule(fw, layer, "--table", "filter", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_FWX", "--in-interface", iface, "--out-interface", iface, @@ -649,7 +655,7 @@ iptablesAddForwardAllowCross(virFirewall *fw, virFirewallLayer layer, const char *iface) { - iptablesForwardAllowCross(fw, layer, iface, ADD); + iptablesForwardAllowCross(fw, layer, iface, VIR_NETFILTER_INSERT); } /** @@ -668,7 +674,7 @@ iptablesRemoveForwardAllowCross(virFirewall *fw, virFirewallLayer layer, const char *iface) { - iptablesForwardAllowCross(fw, layer, iface, REMOVE); + iptablesForwardAllowCross(fw, layer, iface, VIR_NETFILTER_DELETE); } static void @@ -679,7 +685,7 @@ iptablesForwardRejectOut(virFirewall *fw, { virFirewallAddRule(fw, layer, "--table", "filter", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_FWO", "--in-interface", iface, "--jump", "REJECT", @@ -701,7 +707,7 @@ iptablesAddForwardRejectOut(virFirewall *fw, virFirewallLayer layer, const char *iface) { - iptablesForwardRejectOut(fw, layer, iface, ADD); + iptablesForwardRejectOut(fw, layer, iface, VIR_NETFILTER_INSERT); } /** @@ -719,7 +725,7 @@ iptablesRemoveForwardRejectOut(virFirewall *fw, virFirewallLayer layer, const char *iface) { - iptablesForwardRejectOut(fw, layer, iface, REMOVE); + iptablesForwardRejectOut(fw, layer, iface, VIR_NETFILTER_DELETE); } @@ -731,7 +737,7 @@ iptablesForwardRejectIn(virFirewall *fw, { virFirewallAddRule(fw, layer, "--table", "filter", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_FWI", "--out-interface", iface, "--jump", "REJECT", @@ -753,7 +759,7 @@ iptablesAddForwardRejectIn(virFirewall *fw, virFirewallLayer layer, const char *iface) { - iptablesForwardRejectIn(fw, layer, iface, ADD); + iptablesForwardRejectIn(fw, layer, iface, VIR_NETFILTER_INSERT); } /** @@ -771,7 +777,7 @@ iptablesRemoveForwardRejectIn(virFirewall *fw, virFirewallLayer layer, const char *iface) { - iptablesForwardRejectIn(fw, layer, iface, REMOVE); + iptablesForwardRejectIn(fw, layer, iface, VIR_NETFILTER_DELETE); } @@ -813,7 +819,7 @@ iptablesForwardMasquerade(virFirewall *fw, if (protocol && protocol[0]) { rule = virFirewallAddRule(fw, layer, "--table", "nat", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_PRT", "--source", networkstr, "-p", protocol, @@ -822,7 +828,7 @@ iptablesForwardMasquerade(virFirewall *fw, } else { rule = virFirewallAddRule(fw, layer, "--table", "nat", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_PRT", "--source", networkstr, "!", "--destination", networkstr, @@ -896,7 +902,8 @@ iptablesAddForwardMasquerade(virFirewall *fw, const char *protocol) { return iptablesForwardMasquerade(fw, netaddr, prefix, - physdev, addr, port, protocol, ADD); + physdev, addr, port, protocol, + VIR_NETFILTER_INSERT); } /** @@ -922,7 +929,8 @@ iptablesRemoveForwardMasquerade(virFirewall *fw, const char *protocol) { return iptablesForwardMasquerade(fw, netaddr, prefix, - physdev, addr, port, protocol, REMOVE); + physdev, addr, port, protocol, + VIR_NETFILTER_DELETE); } @@ -947,7 +955,7 @@ iptablesForwardDontMasquerade(virFirewall *fw, if (physdev && physdev[0]) virFirewallAddRule(fw, layer, "--table", "nat", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_PRT", "--out-interface", physdev, "--source", networkstr, @@ -957,7 +965,7 @@ iptablesForwardDontMasquerade(virFirewall *fw, else virFirewallAddRule(fw, layer, "--table", "nat", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_PRT", "--source", networkstr, "--destination", destaddr, @@ -989,7 +997,7 @@ iptablesAddDontMasquerade(virFirewall *fw, const char *destaddr) { return iptablesForwardDontMasquerade(fw, netaddr, prefix, - physdev, destaddr, ADD); + physdev, destaddr, VIR_NETFILTER_INSERT); } /** @@ -1014,7 +1022,8 @@ iptablesRemoveDontMasquerade(virFirewall *fw, const char *destaddr) { return iptablesForwardDontMasquerade(fw, netaddr, prefix, - physdev, destaddr, REMOVE); + physdev, destaddr, + VIR_NETFILTER_DELETE); } @@ -1031,7 +1040,7 @@ iptablesOutputFixUdpChecksum(virFirewall *fw, virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4, "--table", "mangle", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_PRT", "--out-interface", iface, "--protocol", "udp", @@ -1056,7 +1065,7 @@ iptablesAddOutputFixUdpChecksum(virFirewall *fw, const char *iface, int port) { - iptablesOutputFixUdpChecksum(fw, iface, port, ADD); + iptablesOutputFixUdpChecksum(fw, iface, port, VIR_NETFILTER_INSERT); } /** @@ -1073,5 +1082,5 @@ iptablesRemoveOutputFixUdpChecksum(virFirewall *fw, const char *iface, int port) { - iptablesOutputFixUdpChecksum(fw, iface, port, REMOVE); + iptablesOutputFixUdpChecksum(fw, iface, port, VIR_NETFILTER_DELETE); } -- 2.33.1
When libvirt added support for firewalld, all iptables/ebtables rules were added via the firewalld "passthrough" API when firewalld was enabled (the "firewalld backend"), or run directly by libvirt when firewalld was disabled (the so-called "direct backend"). virfirewalltest.c dutifully ran each test twice, once with the each backend enabled. But commit b19863640d changed the code to *always* directly run iptables/ebtables commands, and never use the firewalld passthrough API, effectively making the direct and firewalld backends identical, except that when libvirt receives notice that firewalld has restarted or reloaded its rules, the firewalld backend sends an extra "iptables -V" command via firewalld's passthrough API (and waits for a response) prior to running all the rest of the iptables commands directly; this assures that a newly-restarted firewalld has finished its work on the filter tables before libvirt starts messing with it. (Because this code is only executed in response to an event from dbus, it isn't tested in the unit tests). In spite of this, we still go through all the virfirewall tests twice though - once for the direct backend, and once for the firewalld backend, even though these take the same codepath. In commit b19863640d I had left this double-testing in thinking that someday we might go back to actually doing something useful with the firewalld backend in the course of adding support for native nftables, but I've now realized that for the case of nftables we will be *even more* divorced from firewalld, so there is really no point in keeping this code around any longer. (It's likely/probable that the tests will be done twice again in the future, but it will be enough different that it is better to remove this code and re-implement from scratch when adding the nftables backend, rather than trying to directly modify the existing code and end up with something even more confusing). This patch eliminates all the test duplication in virfirewalltest.c, including mocking dbus, which is unnecessary since none of the tests use dbus (for now we ensure that by explicitly setting the virfirewall backend to DIRECT before any of the tests have run. Eventually the concept of a "firewalld backend" will disappear completely, but that's for another patch.) Signed-off-by: Laine Stump <laine@redhat.com> --- tests/virfirewalltest.c | 293 +++------------------------------------- 1 file changed, 20 insertions(+), 273 deletions(-) diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index c6f4ca05e2..e6c41d89fa 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -35,10 +35,6 @@ # define VIR_FROM_THIS VIR_FROM_FIREWALL -static bool fwDisabled = true; -static virBuffer *fwBuf; -static bool fwError; - # define TEST_FILTER_TABLE_LIST \ "Chain INPUT (policy ACCEPT)\n" \ "target prot opt source destination\n" \ @@ -62,124 +58,9 @@ static bool fwError; "Chain POSTROUTING (policy ACCEPT)\n" \ "target prot opt source destination\n" -VIR_MOCK_WRAP_RET_ARGS(g_dbus_connection_call_sync, - GVariant *, - GDBusConnection *, connection, - const gchar *, bus_name, - const gchar *, object_path, - const gchar *, interface_name, - const gchar *, method_name, - GVariant *, parameters, - const GVariantType *, reply_type, - GDBusCallFlags, flags, - gint, timeout_msec, - GCancellable *, cancellable, - GError **, error) -{ - GVariant *reply = NULL; - g_autoptr(GVariant) params = parameters; - - if (params) - g_variant_ref_sink(params); - - VIR_MOCK_REAL_INIT(g_dbus_connection_call_sync); - - if (STREQ(bus_name, "org.freedesktop.DBus") && - STREQ(method_name, "ListNames")) { - GVariantBuilder builder; - - g_variant_builder_init(&builder, G_VARIANT_TYPE("(as)")); - g_variant_builder_open(&builder, G_VARIANT_TYPE("as")); - - g_variant_builder_add(&builder, "s", "org.foo.bar.wizz"); - - if (!fwDisabled) - g_variant_builder_add(&builder, "s", VIR_FIREWALL_FIREWALLD_SERVICE); - - g_variant_builder_close(&builder); - - reply = g_variant_builder_end(&builder); - } else if (STREQ(bus_name, VIR_FIREWALL_FIREWALLD_SERVICE) && - STREQ(method_name, "passthrough")) { - g_autoptr(GVariantIter) iter = NULL; - static const size_t maxargs = 5; - g_auto(GStrv) args = NULL; - size_t nargs = 0; - char *type = NULL; - char *item = NULL; - bool isAdd = false; - bool doError = false; - - g_variant_get(params, "(&sas)", &type, &iter); - - args = g_new0(char *, maxargs); - - if (fwBuf) { - if (STREQ(type, "ipv4")) - virBufferAddLit(fwBuf, IPTABLES); - else if (STREQ(type, "ipv6")) - virBufferAddLit(fwBuf, IP6TABLES); - else - virBufferAddLit(fwBuf, EBTABLES); - } - - while (g_variant_iter_loop(iter, "s", &item)) { - /* Fake failure on the command with this IP addr */ - if (STREQ(item, "-A")) { - isAdd = true; - } else if (isAdd && STREQ(item, "192.168.122.255")) { - doError = true; - } - - if (nargs < maxargs) - args[nargs] = g_strdup(item); - nargs++; - - if (fwBuf) { - virBufferAddLit(fwBuf, " "); - virBufferEscapeShell(fwBuf, item); - } - } - - if (fwBuf) - virBufferAddLit(fwBuf, "\n"); - - if (doError) { - if (error) - *error = g_dbus_error_new_for_dbus_error("org.firewalld.error", - "something bad happened"); - } else { - if (nargs == 2 && - STREQ(type, "ipv4") && - STREQ(args[0], "-w") && - STREQ(args[1], "-L")) { - reply = g_variant_new("(s)", TEST_FILTER_TABLE_LIST); - } else if (nargs == 4 && - STREQ(type, "ipv4") && - STREQ(args[0], "-w") && - STREQ(args[1], "-t") && - STREQ(args[2], "nat") && - STREQ(args[3], "-L")) { - reply = g_variant_new("(s)", TEST_NAT_TABLE_LIST); - } else { - reply = g_variant_new("(s)", "success"); - } - } - } else { - reply = g_variant_new("()"); - } - - return reply; -} - -struct testFirewallData { - virFirewallBackend tryBackend; - virFirewallBackend expectBackend; - bool fwDisabled; -}; static int -testFirewallSingleGroup(const void *opaque) +testFirewallSingleGroup(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); @@ -188,18 +69,10 @@ testFirewallSingleGroup(const void *opaque) const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" IPTABLES " -w -A INPUT --source '!192.168.122.1' --jump REJECT\n"; - const struct testFirewallData *data = opaque; - g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); - fwDisabled = data->fwDisabled; - if (virFirewallSetBackend(data->tryBackend) < 0) - goto cleanup; + g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); - if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT || - data->expectBackend == VIR_FIREWALL_BACKEND_FIREWALLD) - virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, NULL, NULL); - else - fwBuf = &cmdbuf; + virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, NULL, NULL); virFirewallStartTransaction(fw, 0); @@ -226,13 +99,12 @@ testFirewallSingleGroup(const void *opaque) ret = 0; cleanup: - fwBuf = NULL; return ret; } static int -testFirewallRemoveRule(const void *opaque) +testFirewallRemoveRule(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); @@ -241,19 +113,10 @@ testFirewallRemoveRule(const void *opaque) const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" IPTABLES " -w -A INPUT --source '!192.168.122.1' --jump REJECT\n"; - const struct testFirewallData *data = opaque; virFirewallRule *fwrule; g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); - fwDisabled = data->fwDisabled; - if (virFirewallSetBackend(data->tryBackend) < 0) - goto cleanup; - - if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT || - data->expectBackend == VIR_FIREWALL_BACKEND_FIREWALLD) - virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, NULL, NULL); - else - fwBuf = &cmdbuf; + virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, NULL, NULL); virFirewallStartTransaction(fw, 0); @@ -286,7 +149,6 @@ testFirewallRemoveRule(const void *opaque) ret = 0; cleanup: - fwBuf = NULL; return ret; } @@ -303,18 +165,9 @@ testFirewallManyGroups(const void *opaque G_GNUC_UNUSED) IPTABLES " -w -A INPUT --source '!192.168.122.1' --jump REJECT\n" IPTABLES " -w -A OUTPUT --source 192.168.122.1 --jump ACCEPT\n" IPTABLES " -w -A OUTPUT --jump DROP\n"; - const struct testFirewallData *data = opaque; g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); - fwDisabled = data->fwDisabled; - if (virFirewallSetBackend(data->tryBackend) < 0) - goto cleanup; - - if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT || - data->expectBackend == VIR_FIREWALL_BACKEND_FIREWALLD) - virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, NULL, NULL); - else - fwBuf = &cmdbuf; + virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, NULL, NULL); virFirewallStartTransaction(fw, 0); @@ -353,7 +206,6 @@ testFirewallManyGroups(const void *opaque G_GNUC_UNUSED) ret = 0; cleanup: - fwBuf = NULL; return ret; } @@ -391,20 +243,9 @@ testFirewallIgnoreFailGroup(const void *opaque G_GNUC_UNUSED) IPTABLES " -w -A INPUT --source 192.168.122.255 --jump REJECT\n" IPTABLES " -w -A OUTPUT --source 192.168.122.1 --jump ACCEPT\n" IPTABLES " -w -A OUTPUT --jump DROP\n"; - const struct testFirewallData *data = opaque; g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); - fwDisabled = data->fwDisabled; - if (virFirewallSetBackend(data->tryBackend) < 0) - goto cleanup; - - if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT || - data->expectBackend == VIR_FIREWALL_BACKEND_FIREWALLD) { - virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, testFirewallRollbackHook, NULL); - } else { - fwBuf = &cmdbuf; - fwError = true; - } + virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, testFirewallRollbackHook, NULL); virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); @@ -443,7 +284,6 @@ testFirewallIgnoreFailGroup(const void *opaque G_GNUC_UNUSED) ret = 0; cleanup: - fwBuf = NULL; return ret; } @@ -460,20 +300,9 @@ testFirewallIgnoreFailRule(const void *opaque G_GNUC_UNUSED) IPTABLES " -w -A INPUT --source 192.168.122.255 --jump REJECT\n" IPTABLES " -w -A OUTPUT --source 192.168.122.1 --jump ACCEPT\n" IPTABLES " -w -A OUTPUT --jump DROP\n"; - const struct testFirewallData *data = opaque; g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); - fwDisabled = data->fwDisabled; - if (virFirewallSetBackend(data->tryBackend) < 0) - goto cleanup; - - if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT || - data->expectBackend == VIR_FIREWALL_BACKEND_FIREWALLD) { - virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, testFirewallRollbackHook, NULL); - } else { - fwBuf = &cmdbuf; - fwError = true; - } + virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, testFirewallRollbackHook, NULL); virFirewallStartTransaction(fw, 0); @@ -511,7 +340,6 @@ testFirewallIgnoreFailRule(const void *opaque G_GNUC_UNUSED) ret = 0; cleanup: - fwBuf = NULL; return ret; } @@ -526,20 +354,9 @@ testFirewallNoRollback(const void *opaque G_GNUC_UNUSED) const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" IPTABLES " -w -A INPUT --source 192.168.122.255 --jump REJECT\n"; - const struct testFirewallData *data = opaque; g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); - fwDisabled = data->fwDisabled; - if (virFirewallSetBackend(data->tryBackend) < 0) - goto cleanup; - - if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT || - data->expectBackend == VIR_FIREWALL_BACKEND_FIREWALLD) { - virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, testFirewallRollbackHook, NULL); - } else { - fwBuf = &cmdbuf; - fwError = true; - } + virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, testFirewallRollbackHook, NULL); virFirewallStartTransaction(fw, 0); @@ -573,7 +390,6 @@ testFirewallNoRollback(const void *opaque G_GNUC_UNUSED) ret = 0; cleanup: - fwBuf = NULL; return ret; } @@ -590,20 +406,9 @@ testFirewallSingleRollback(const void *opaque G_GNUC_UNUSED) IPTABLES " -w -D INPUT --source 192.168.122.1 --jump ACCEPT\n" IPTABLES " -w -D INPUT --source 192.168.122.255 --jump REJECT\n" IPTABLES " -w -D INPUT --source '!192.168.122.1' --jump REJECT\n"; - const struct testFirewallData *data = opaque; g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); - fwDisabled = data->fwDisabled; - if (virFirewallSetBackend(data->tryBackend) < 0) - goto cleanup; - - if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT || - data->expectBackend == VIR_FIREWALL_BACKEND_FIREWALLD) { - virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, testFirewallRollbackHook, NULL); - } else { - fwError = true; - fwBuf = &cmdbuf; - } + virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, testFirewallRollbackHook, NULL); virFirewallStartTransaction(fw, 0); @@ -654,7 +459,6 @@ testFirewallSingleRollback(const void *opaque G_GNUC_UNUSED) ret = 0; cleanup: - fwBuf = NULL; return ret; } @@ -670,20 +474,9 @@ testFirewallManyRollback(const void *opaque G_GNUC_UNUSED) IPTABLES " -w -A INPUT --source 192.168.122.255 --jump REJECT\n" IPTABLES " -w -D INPUT --source 192.168.122.255 --jump REJECT\n" IPTABLES " -w -D INPUT --source '!192.168.122.1' --jump REJECT\n"; - const struct testFirewallData *data = opaque; g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); - fwDisabled = data->fwDisabled; - if (virFirewallSetBackend(data->tryBackend) < 0) - goto cleanup; - - if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT || - data->expectBackend == VIR_FIREWALL_BACKEND_FIREWALLD) { - virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, testFirewallRollbackHook, NULL); - } else { - fwBuf = &cmdbuf; - fwError = true; - } + virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, testFirewallRollbackHook, NULL); virFirewallStartTransaction(fw, 0); @@ -738,7 +531,6 @@ testFirewallManyRollback(const void *opaque G_GNUC_UNUSED) ret = 0; cleanup: - fwBuf = NULL; return ret; } @@ -758,20 +550,9 @@ testFirewallChainedRollback(const void *opaque G_GNUC_UNUSED) IPTABLES " -w -D INPUT --source '!192.168.122.1' --jump REJECT\n" IPTABLES " -w -D INPUT --source 192.168.122.255 --jump REJECT\n" IPTABLES " -w -D INPUT --source '!192.168.122.1' --jump REJECT\n"; - const struct testFirewallData *data = opaque; g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); - fwDisabled = data->fwDisabled; - if (virFirewallSetBackend(data->tryBackend) < 0) - goto cleanup; - - if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT || - data->expectBackend == VIR_FIREWALL_BACKEND_FIREWALLD) { - virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, testFirewallRollbackHook, NULL); - } else { - fwBuf = &cmdbuf; - fwError = true; - } + virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, testFirewallRollbackHook, NULL); virFirewallStartTransaction(fw, 0); @@ -852,7 +633,6 @@ testFirewallChainedRollback(const void *opaque G_GNUC_UNUSED) ret = 0; cleanup: - fwBuf = NULL; return ret; } @@ -952,22 +732,12 @@ testFirewallQuery(const void *opaque G_GNUC_UNUSED) IPTABLES " -w -A INPUT --source '!192.168.122.129' --jump REJECT\n" IPTABLES " -w -A INPUT --source 192.168.122.128 --jump REJECT\n" IPTABLES " -w -A INPUT --source '!192.168.122.1' --jump REJECT\n"; - const struct testFirewallData *data = opaque; g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); expectedLineNum = 0; expectedLineError = false; - fwDisabled = data->fwDisabled; - if (virFirewallSetBackend(data->tryBackend) < 0) - goto cleanup; - if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT || - data->expectBackend == VIR_FIREWALL_BACKEND_FIREWALLD) { - virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, testFirewallQueryHook, NULL); - } else { - fwBuf = &cmdbuf; - fwError = true; - } + virCommandSetDryRun(dryRunToken, &cmdbuf, false, false, testFirewallQueryHook, NULL); virFirewallStartTransaction(fw, 0); @@ -1030,7 +800,6 @@ testFirewallQuery(const void *opaque G_GNUC_UNUSED) ret = 0; cleanup: - fwBuf = NULL; return ret; } @@ -1040,40 +809,15 @@ mymain(void) { int ret = 0; -# define RUN_TEST_DIRECT(name, method) \ - do { \ - struct testFirewallData data; \ - data.tryBackend = VIR_FIREWALL_BACKEND_AUTOMATIC; \ - data.expectBackend = VIR_FIREWALL_BACKEND_DIRECT; \ - data.fwDisabled = true; \ - if (virTestRun(name " auto direct", method, &data) < 0) \ - ret = -1; \ - data.tryBackend = VIR_FIREWALL_BACKEND_DIRECT; \ - data.expectBackend = VIR_FIREWALL_BACKEND_DIRECT; \ - data.fwDisabled = true; \ - if (virTestRun(name " manual direct", method, &data) < 0) \ - ret = -1; \ - } while (0) + if (virFirewallSetBackend(VIR_FIREWALL_BACKEND_DIRECT) < 0) + return EXIT_FAILURE; -# define RUN_TEST_FIREWALLD(name, method) \ +# define RUN_TEST(name, method) \ do { \ - struct testFirewallData data; \ - data.tryBackend = VIR_FIREWALL_BACKEND_AUTOMATIC; \ - data.expectBackend = VIR_FIREWALL_BACKEND_FIREWALLD; \ - data.fwDisabled = false; \ - if (virTestRun(name " auto firewalld", method, &data) < 0) \ - ret = -1; \ - data.tryBackend = VIR_FIREWALL_BACKEND_FIREWALLD; \ - data.expectBackend = VIR_FIREWALL_BACKEND_FIREWALLD; \ - data.fwDisabled = false; \ - if (virTestRun(name " manual firewalld", method, &data) < 0) \ + if (virTestRun(name, method, NULL) < 0) \ ret = -1; \ } while (0) -# define RUN_TEST(name, method) \ - RUN_TEST_DIRECT(name, method); \ - RUN_TEST_FIREWALLD(name, method) - RUN_TEST("single group", testFirewallSingleGroup); RUN_TEST("remove rule", testFirewallRemoveRule); RUN_TEST("many groups", testFirewallManyGroups); @@ -1088,8 +832,11 @@ mymain(void) return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } +# if 0 VIR_TEST_MAIN_PRELOAD(mymain, VIR_TEST_MOCK("virgdbus"), VIR_TEST_MOCK("virfirewall")) +# endif +VIR_TEST_MAIN_PRELOAD(mymain, VIR_TEST_MOCK("virfirewall")) #else /* ! defined (__linux__) */ -- 2.33.1
Several functions were simplified to remove the only cleanup code at the cleanup label, making it unnecessary. Signed-off-by: Laine Stump <laine@redhat.com> --- tests/virfirewalltest.c | 92 ++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 61 deletions(-) diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index e6c41d89fa..724d3081f1 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -64,7 +64,6 @@ testFirewallSingleGroup(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); - int ret = -1; const char *actual = NULL; const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" @@ -87,19 +86,17 @@ testFirewallSingleGroup(const void *opaque G_GNUC_UNUSED) "--jump", "REJECT", NULL); if (virFirewallApply(fw) < 0) - goto cleanup; + return -1; actual = virBufferCurrentContent(&cmdbuf); if (STRNEQ_NULLABLE(expected, actual)) { fprintf(stderr, "Unexpected command execution\n"); virTestDifference(stderr, expected, actual); - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } @@ -108,7 +105,6 @@ testFirewallRemoveRule(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); - int ret = -1; const char *actual = NULL; const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" @@ -137,19 +133,17 @@ testFirewallRemoveRule(const void *opaque G_GNUC_UNUSED) virFirewallRuleAddArgList(fw, fwrule, "--jump", "REJECT", NULL); if (virFirewallApply(fw) < 0) - goto cleanup; + return -1; actual = virBufferCurrentContent(&cmdbuf); if (STRNEQ_NULLABLE(expected, actual)) { fprintf(stderr, "Unexpected command execution\n"); virTestDifference(stderr, expected, actual); - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } @@ -158,7 +152,6 @@ testFirewallManyGroups(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); - int ret = -1; const char *actual = NULL; const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" @@ -194,19 +187,17 @@ testFirewallManyGroups(const void *opaque G_GNUC_UNUSED) if (virFirewallApply(fw) < 0) - goto cleanup; + return -1; actual = virBufferCurrentContent(&cmdbuf); if (STRNEQ_NULLABLE(expected, actual)) { fprintf(stderr, "Unexpected command execution\n"); virTestDifference(stderr, expected, actual); - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } static void @@ -236,7 +227,6 @@ testFirewallIgnoreFailGroup(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); - int ret = -1; const char *actual = NULL; const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" @@ -272,19 +262,17 @@ testFirewallIgnoreFailGroup(const void *opaque G_GNUC_UNUSED) if (virFirewallApply(fw) < 0) - goto cleanup; + return -1; actual = virBufferCurrentContent(&cmdbuf); if (STRNEQ_NULLABLE(expected, actual)) { fprintf(stderr, "Unexpected command execution\n"); virTestDifference(stderr, expected, actual); - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } @@ -293,7 +281,6 @@ testFirewallIgnoreFailRule(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); - int ret = -1; const char *actual = NULL; const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" @@ -328,19 +315,17 @@ testFirewallIgnoreFailRule(const void *opaque G_GNUC_UNUSED) if (virFirewallApply(fw) < 0) - goto cleanup; + return -1; actual = virBufferCurrentContent(&cmdbuf); if (STRNEQ_NULLABLE(expected, actual)) { fprintf(stderr, "Unexpected command execution\n"); virTestDifference(stderr, expected, actual); - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } @@ -349,7 +334,6 @@ testFirewallNoRollback(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); - int ret = -1; const char *actual = NULL; const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" @@ -377,7 +361,7 @@ testFirewallNoRollback(const void *opaque G_GNUC_UNUSED) if (virFirewallApply(fw) == 0) { fprintf(stderr, "Firewall apply unexpectedly worked\n"); - goto cleanup; + return -1; } actual = virBufferCurrentContent(&cmdbuf); @@ -385,12 +369,10 @@ testFirewallNoRollback(const void *opaque G_GNUC_UNUSED) if (STRNEQ_NULLABLE(expected, actual)) { fprintf(stderr, "Unexpected command execution\n"); virTestDifference(stderr, expected, actual); - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } static int @@ -398,7 +380,6 @@ testFirewallSingleRollback(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); - int ret = -1; const char *actual = NULL; const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" @@ -446,7 +427,7 @@ testFirewallSingleRollback(const void *opaque G_GNUC_UNUSED) if (virFirewallApply(fw) == 0) { fprintf(stderr, "Firewall apply unexpectedly worked\n"); - goto cleanup; + return -1; } actual = virBufferCurrentContent(&cmdbuf); @@ -454,12 +435,10 @@ testFirewallSingleRollback(const void *opaque G_GNUC_UNUSED) if (STRNEQ_NULLABLE(expected, actual)) { fprintf(stderr, "Unexpected command execution\n"); virTestDifference(stderr, expected, actual); - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } static int @@ -467,7 +446,6 @@ testFirewallManyRollback(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); - int ret = -1; const char *actual = NULL; const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" @@ -518,7 +496,7 @@ testFirewallManyRollback(const void *opaque G_GNUC_UNUSED) if (virFirewallApply(fw) == 0) { fprintf(stderr, "Firewall apply unexpectedly worked\n"); - goto cleanup; + return -1; } actual = virBufferCurrentContent(&cmdbuf); @@ -526,12 +504,10 @@ testFirewallManyRollback(const void *opaque G_GNUC_UNUSED) if (STRNEQ_NULLABLE(expected, actual)) { fprintf(stderr, "Unexpected command execution\n"); virTestDifference(stderr, expected, actual); - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } static int @@ -539,7 +515,6 @@ testFirewallChainedRollback(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); - int ret = -1; const char *actual = NULL; const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" @@ -620,7 +595,7 @@ testFirewallChainedRollback(const void *opaque G_GNUC_UNUSED) if (virFirewallApply(fw) == 0) { fprintf(stderr, "Firewall apply unexpectedly worked\n"); - goto cleanup; + return -1; } actual = virBufferCurrentContent(&cmdbuf); @@ -628,12 +603,10 @@ testFirewallChainedRollback(const void *opaque G_GNUC_UNUSED) if (STRNEQ_NULLABLE(expected, actual)) { fprintf(stderr, "Unexpected command execution\n"); virTestDifference(stderr, expected, actual); - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } @@ -720,7 +693,6 @@ testFirewallQuery(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); - int ret = -1; const char *actual = NULL; const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" @@ -783,24 +755,22 @@ testFirewallQuery(const void *opaque G_GNUC_UNUSED) "--jump", "REJECT", NULL); if (virFirewallApply(fw) < 0) - goto cleanup; + return -1; actual = virBufferCurrentContent(&cmdbuf); if (expectedLineError) { fprintf(stderr, "Got some unexpected query data\n"); - goto cleanup; + return -1; } if (STRNEQ_NULLABLE(expected, actual)) { fprintf(stderr, "Unexpected command execution\n"); virTestDifference(stderr, expected, actual); - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } -- 2.33.1
It isn't intuitive (to me) that a test just converting xml text into iptables commands should need to call dbus, so rather than forcing the next person to look through the commit logs and/or run the test under gdb to understand why this is needed, just add a short comment in the source. Signed-off-by: Laine Stump <laine@redhat.com> --- tests/networkxml2firewalltest.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c index e4f86bc3fc..68a82e60d6 100644 --- a/tests/networkxml2firewalltest.c +++ b/tests/networkxml2firewalltest.c @@ -187,6 +187,12 @@ mymain(void) return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } +/* NB: virgdbus must be mocked because this test calls + * networkAddFirewallRules(), which will always call + * virFirewallDIsRegistered(), which calls + * virGDBusIsServiceRegistered(). + */ + VIR_TEST_MAIN_PRELOAD(mymain, VIR_TEST_MOCK("virgdbus"), VIR_TEST_MOCK("virfirewall")) -- 2.33.1
Since commit b19863640 both useful cases of the switch statement in this function have made the same call (and the other/default case is just an error that can never happen). Eliminate the switch to help eliminate use of currentBackend. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virfirewall.c | 25 +------------------------ 1 file changed, 1 insertion(+), 24 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 1a546335f6..bb14a367d9 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -653,31 +653,8 @@ virFirewallApplyRule(virFirewall *firewall, if (rule->ignoreErrors) ignoreErrors = rule->ignoreErrors; - switch (currentBackend) { - case VIR_FIREWALL_BACKEND_DIRECT: - if (virFirewallApplyRuleDirect(rule, ignoreErrors, &output) < 0) - return -1; - break; - case VIR_FIREWALL_BACKEND_FIREWALLD: - /* Since we are using raw iptables rules, there is no - * advantage to going through firewalld, so instead just add - * them directly rather that via dbus calls to firewalld. This - * has the useful side effect of eliminating extra unwanted - * warning messages in the system logs when trying to delete - * rules that don't exist (which is something that happens - * often when libvirtd is started, and *always* when firewalld - * is restarted) - */ - if (virFirewallApplyRuleDirect(rule, ignoreErrors, &output) < 0) - return -1; - break; - - case VIR_FIREWALL_BACKEND_AUTOMATIC: - case VIR_FIREWALL_BACKEND_LAST: - default: - virReportEnumRangeError(virFirewallBackend, currentBackend); + if (virFirewallApplyRuleDirect(rule, ignoreErrors, &output) < 0) return -1; - } if (rule->queryCB && output) { if (!(lines = g_strsplit(output, "\n", -1))) -- 2.33.1
This function doesn't need to check for a backend - synchronization with firewalld should always be done whenever firewalld is registered and available, not just when the firewalld backend is selected. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virfirewall.c | 54 ++++++++++++++++++++++++++---------------- src/util/viriptables.c | 6 ++--- 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index bb14a367d9..2fc9f94729 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -616,27 +616,41 @@ virFirewallBackendSynchronize(void) { const char *arg = "-V"; g_autofree char *output = NULL; + int firewallDRegistered = virFirewallDIsRegistered(); + + /* + * virFirewallBackendSynchronize() should be called after + * receiving an ownership-change event or reload event for + * firewalld from dbus, prior to performing any operations on the + * default table "filter". + * + * Our iptables filter rules are added to (private chains within) + * the default table named "filter", which is flushed by firewalld + * any time it is restarted or reloads its rules. libvirt watches + * for notifications that firewalld has been restarted / its rules + * reloaded, and then reloads the libvirt rules. But it's possible + * for libvirt to be notified that firewalld has restarted prior + * to firewalld completing initialization, and when that race + * happens, firewalld can potentially flush out rules that libvirt + * has just added! + * + * To prevent this, we send a simple command ("iptables -V") via + * firewalld's passthrough iptables API, and wait until it's + * finished before sending our own directly-executed iptables + * commands. This assures that firewalld has fully initialized and + * caught up with its internal queue of iptables commands, and + * won't stomp all over the new rules we subsequently add. + * + */ - switch (currentBackend) { - case VIR_FIREWALL_BACKEND_DIRECT: - /* nobody to synchronize with */ - break; - case VIR_FIREWALL_BACKEND_FIREWALLD: - /* Send a simple rule via firewalld's passthrough iptables - * command so that we'll be sure firewalld has fully - * initialized and caught up with its internal queue of - * iptables commands. Waiting for this will prevent our own - * directly-executed iptables commands from being run while - * firewalld is still initializing. - */ - ignore_value(virFirewallDApplyRule(VIR_FIREWALL_LAYER_IPV4, - (char **)&arg, 1, true, &output)); - VIR_DEBUG("Result of 'iptables -V' via firewalld: %s", NULLSTR(output)); - break; - case VIR_FIREWALL_BACKEND_AUTOMATIC: - case VIR_FIREWALL_BACKEND_LAST: - break; - } + VIR_DEBUG("Firewalld is registered ? %d", firewallDRegistered); + + if (firewallDRegistered < 0) + return; /* firewalld (or dbus?) not functional, don't sync */ + + ignore_value(virFirewallDApplyRule(VIR_FIREWALL_LAYER_IPV4, + (char **)&arg, 1, true, &output)); + VIR_DEBUG("Result of 'iptables -V' via firewalld: %s", NULLSTR(output)); } diff --git a/src/util/viriptables.c b/src/util/viriptables.c index d2bc10a652..34ce9cd018 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -138,10 +138,10 @@ iptablesSetupPrivateChains(virFirewallLayer layer) }; size_t i; - /* When the backend is firewalld, we need to make sure that + /* When firewalld.service is active, we need to make sure that * firewalld has been fully started and completed its - * initialization, otherwise firewalld might delete our rules soon - * after we add them! + * initialization, otherwise it might delete our rules soon after + * we add them! */ virFirewallBackendSynchronize(); -- 2.33.1
This function doesn't have anything to do with manipulating virFirewall objects, but rather should be called in response to dbus events about the firewalld service. Move this function into virfirewalld.c, and rename it to virFirewallDSynchronize(). Signed-off-by: Laine Stump <laine@redhat.com> --- src/libvirt_private.syms | 2 +- src/util/virfirewall.c | 43 ---------------------------------------- src/util/virfirewall.h | 2 -- src/util/virfirewalld.c | 43 ++++++++++++++++++++++++++++++++++++++++ src/util/virfirewalld.h | 2 ++ src/util/viriptables.c | 3 ++- 6 files changed, 48 insertions(+), 47 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 72b38a970d..23385ec7a1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2308,7 +2308,6 @@ virFileCacheSetPriv; # util/virfirewall.h virFirewallAddRuleFull; virFirewallApply; -virFirewallBackendSynchronize; virFirewallFree; virFirewallNew; virFirewallRemoveRule; @@ -2329,6 +2328,7 @@ virFirewallDGetVersion; virFirewallDGetZones; virFirewallDInterfaceSetZone; virFirewallDIsRegistered; +virFirewallDSynchronize; virFirewallDZoneExists; diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 2fc9f94729..f3172e5c96 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -611,49 +611,6 @@ virFirewallApplyRuleFirewallD(virFirewallRule *rule, } -void -virFirewallBackendSynchronize(void) -{ - const char *arg = "-V"; - g_autofree char *output = NULL; - int firewallDRegistered = virFirewallDIsRegistered(); - - /* - * virFirewallBackendSynchronize() should be called after - * receiving an ownership-change event or reload event for - * firewalld from dbus, prior to performing any operations on the - * default table "filter". - * - * Our iptables filter rules are added to (private chains within) - * the default table named "filter", which is flushed by firewalld - * any time it is restarted or reloads its rules. libvirt watches - * for notifications that firewalld has been restarted / its rules - * reloaded, and then reloads the libvirt rules. But it's possible - * for libvirt to be notified that firewalld has restarted prior - * to firewalld completing initialization, and when that race - * happens, firewalld can potentially flush out rules that libvirt - * has just added! - * - * To prevent this, we send a simple command ("iptables -V") via - * firewalld's passthrough iptables API, and wait until it's - * finished before sending our own directly-executed iptables - * commands. This assures that firewalld has fully initialized and - * caught up with its internal queue of iptables commands, and - * won't stomp all over the new rules we subsequently add. - * - */ - - VIR_DEBUG("Firewalld is registered ? %d", firewallDRegistered); - - if (firewallDRegistered < 0) - return; /* firewalld (or dbus?) not functional, don't sync */ - - ignore_value(virFirewallDApplyRule(VIR_FIREWALL_LAYER_IPV4, - (char **)&arg, 1, true, &output)); - VIR_DEBUG("Result of 'iptables -V' via firewalld: %s", NULLSTR(output)); -} - - static int virFirewallApplyRule(virFirewall *firewall, virFirewallRule *rule, diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h index 169d99fe2b..7448825dbc 100644 --- a/src/util/virfirewall.h +++ b/src/util/virfirewall.h @@ -109,6 +109,4 @@ void virFirewallStartRollback(virFirewall *firewall, int virFirewallApply(virFirewall *firewall); -void virFirewallBackendSynchronize(void); - G_DEFINE_AUTOPTR_CLEANUP_FUNC(virFirewall, virFirewallFree); diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c index 3178bf4b3d..4795bf7925 100644 --- a/src/util/virfirewalld.c +++ b/src/util/virfirewalld.c @@ -368,3 +368,46 @@ virFirewallDInterfaceSetZone(const char *iface, "changeZoneOfInterface", message); } + + +void +virFirewallDSynchronize(void) +{ + const char *arg = "-V"; + g_autofree char *output = NULL; + int firewallDRegistered = virFirewallDIsRegistered(); + + /* + * virFirewallDSynchronize() should be called after receiving an + * ownership-change event or reload event for firewalld from dbus, + * prior to performing any operations on the default table + * "filter". + * + * Our iptables filter rules are added to (private chains within) + * the default table named "filter", which is flushed by firewalld + * any time it is restarted or reloads its rules. libvirt watches + * for notifications that firewalld has been restarted / its rules + * reloaded, and then reloads the libvirt rules. But it's possible + * for libvirt to be notified that firewalld has restarted prior + * to firewalld completing initialization, and when that race + * happens, firewalld can potentially flush out rules that libvirt + * has just added! + * + * To prevent this, we send a simple command ("iptables -V") via + * firewalld's passthrough iptables API, and wait until it's + * finished before sending our own directly-executed iptables + * commands. This assures that firewalld has fully initialized and + * caught up with its internal queue of iptables commands, and + * won't stomp all over the new rules we subsequently add. + * + */ + + VIR_DEBUG("Firewalld is registered ? %d", firewallDRegistered); + + if (firewallDRegistered < 0) + return; /* firewalld (or dbus?) not functional, don't sync */ + + ignore_value(virFirewallDApplyRule(VIR_FIREWALL_LAYER_IPV4, + (char **)&arg, 1, true, &output)); + VIR_DEBUG("Result of 'iptables -V' via firewalld: %s", NULLSTR(output)); +} diff --git a/src/util/virfirewalld.h b/src/util/virfirewalld.h index d2db3b6f47..c396802a2f 100644 --- a/src/util/virfirewalld.h +++ b/src/util/virfirewalld.h @@ -41,3 +41,5 @@ int virFirewallDApplyRule(virFirewallLayer layer, int virFirewallDInterfaceSetZone(const char *iface, const char *zone); + +void virFirewallDSynchronize(void); diff --git a/src/util/viriptables.c b/src/util/viriptables.c index 34ce9cd018..7db09a0d80 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -28,6 +28,7 @@ #include "internal.h" #include "viriptables.h" +#include "virfirewalld.h" #include "vircommand.h" #include "viralloc.h" #include "virerror.h" @@ -143,7 +144,7 @@ iptablesSetupPrivateChains(virFirewallLayer layer) * initialization, otherwise it might delete our rules soon after * we add them! */ - virFirewallBackendSynchronize(); + virFirewallDSynchronize(); virFirewallStartTransaction(fw, 0); -- 2.33.1
It's unclear exactly why this check exists; possibly a parallel to a long-removed check for the firewall-cmd binary (added to viriptables.c with the initial support for firewalld in commit bf156385a03 in 2012, and long since removed), or possibly because virFirewallOnceInit() was intended to be called at daemon startup, and it seemed like a good idea to just log this error once when trying to determine whether to use firewalld, or direct iptables commands, and then not waste time building commands that could never be executed. The odd thing is that it would sometimes result in logging an error when it couldn't find a binary that wasn't needed anyway (e.g., if all the rules were iptables rules, but ebtables and/or ip6tables weren't also installed). If we just remove this check, then virCommandRun() will end up logging an error and failing if the needed binary isn't found when we try to execute it, which seems like it should just as good (or at least good enough, especially since we eventually want to get rid of iptables completely). So let's remove it! Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virfirewall.c | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index f3172e5c96..1e6c667ee1 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -98,23 +98,6 @@ VIR_ONCE_GLOBAL_INIT(virFirewall); static int virFirewallValidateBackend(virFirewallBackend backend) { - const char *commands[] = { - IPTABLES, IP6TABLES, EBTABLES - }; - size_t i; - - for (i = 0; i < G_N_ELEMENTS(commands); i++) { - g_autofree char *path = virFindFileInPath(commands[i]); - - if (!path) { - virReportSystemError(errno, - _("%s not available, firewall backend will not function"), - commands[i]); - return -1; - } - } - VIR_DEBUG("found iptables/ip6tables/ebtables"); - if (backend == VIR_FIREWALL_BACKEND_AUTOMATIC || backend == VIR_FIREWALL_BACKEND_FIREWALLD) { int rv = virFirewallDIsRegistered(); @@ -694,14 +677,6 @@ virFirewallApply(virFirewall *firewall) virMutexLock(&ruleLock); - if (currentBackend == VIR_FIREWALL_BACKEND_AUTOMATIC) { - /* a specific backend should have been set when the firewall - * object was created. If not, it means none was found. - */ - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to initialize a valid firewall backend")); - goto cleanup; - } if (!firewall || firewall->err) { int err = EINVAL; -- 2.33.1
Since the currentBackend (direct vs. firewalld) setting is no longer used for anything, we don't need to set it (either explicitly from tests, or implicitly during init), and can completely remove it. Signed-off-by: Laine Stump <laine@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virfirewall.c | 50 ++------------------------------ src/util/virfirewallpriv.h | 37 ----------------------- tests/networkxml2firewalltest.c | 8 +---- tests/nwfilterebiptablestest.c | 7 ----- tests/nwfilterxml2firewalltest.c | 8 +---- tests/virfirewalltest.c | 7 ++--- 7 files changed, 6 insertions(+), 112 deletions(-) delete mode 100644 src/util/virfirewallpriv.h diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 23385ec7a1..bb90659365 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2316,7 +2316,6 @@ virFirewallRuleAddArgFormat; virFirewallRuleAddArgList; virFirewallRuleAddArgSet; virFirewallRuleGetArgCount; -virFirewallSetBackend; virFirewallStartRollback; virFirewallStartTransaction; diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 1e6c667ee1..98d78857df 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -22,8 +22,7 @@ #include <stdarg.h> -#define LIBVIRT_VIRFIREWALLPRIV_H_ALLOW -#include "virfirewallpriv.h" +#include "virfirewall.h" #include "virfirewalld.h" #include "viralloc.h" #include "virerror.h" @@ -81,61 +80,16 @@ struct _virFirewall { size_t currentGroup; }; -static virFirewallBackend currentBackend = VIR_FIREWALL_BACKEND_AUTOMATIC; static virMutex ruleLock = VIR_MUTEX_INITIALIZER; -static int -virFirewallValidateBackend(virFirewallBackend backend); - static int virFirewallOnceInit(void) { - return virFirewallValidateBackend(currentBackend); -} - -VIR_ONCE_GLOBAL_INIT(virFirewall); - -static int -virFirewallValidateBackend(virFirewallBackend backend) -{ - if (backend == VIR_FIREWALL_BACKEND_AUTOMATIC || - backend == VIR_FIREWALL_BACKEND_FIREWALLD) { - int rv = virFirewallDIsRegistered(); - - VIR_DEBUG("Firewalld is registered ? %d", rv); - - if (rv == -1) - return -1; - - if (rv == -2) { - if (backend == VIR_FIREWALL_BACKEND_FIREWALLD) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("firewalld backend requested, but service is not running")); - return -1; - } else { - VIR_DEBUG("firewalld service not running, using direct backend"); - backend = VIR_FIREWALL_BACKEND_DIRECT; - } - } else { - VIR_DEBUG("firewalld service running, using firewalld backend"); - backend = VIR_FIREWALL_BACKEND_FIREWALLD; - } - } - - currentBackend = backend; return 0; } -int -virFirewallSetBackend(virFirewallBackend backend) -{ - currentBackend = backend; - - if (virFirewallInitialize() < 0) - return -1; +VIR_ONCE_GLOBAL_INIT(virFirewall); - return virFirewallValidateBackend(backend); -} static virFirewallGroup * virFirewallGroupNew(void) diff --git a/src/util/virfirewallpriv.h b/src/util/virfirewallpriv.h deleted file mode 100644 index b846f8799c..0000000000 --- a/src/util/virfirewallpriv.h +++ /dev/null @@ -1,37 +0,0 @@ -/* - * virfirewallpriv.h: integration with firewalls private APIs - * - * Copyright (C) 2013 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - */ - -#ifndef LIBVIRT_VIRFIREWALLPRIV_H_ALLOW -# error "virfirewallpriv.h may only be included by virfirewall.c or test suites" -#endif /* LIBVIRT_VIRFIREWALLPRIV_H_ALLOW */ - -#pragma once - -#include "virfirewall.h" - -typedef enum { - VIR_FIREWALL_BACKEND_AUTOMATIC, - VIR_FIREWALL_BACKEND_DIRECT, - VIR_FIREWALL_BACKEND_FIREWALLD, - - VIR_FIREWALL_BACKEND_LAST, -} virFirewallBackend; - -int virFirewallSetBackend(virFirewallBackend backend); diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c index 68a82e60d6..11be85e06f 100644 --- a/tests/networkxml2firewalltest.c +++ b/tests/networkxml2firewalltest.c @@ -31,9 +31,7 @@ # include "network/bridge_driver_platform.h" # include "virbuffer.h" # include "virmock.h" - -# define LIBVIRT_VIRFIREWALLPRIV_H_ALLOW -# include "virfirewallpriv.h" +# include "virfirewall.h" # define LIBVIRT_VIRCOMMANDPRIV_H_ALLOW # include "vircommandpriv.h" @@ -167,10 +165,6 @@ mymain(void) ret = -1; \ } while (0) - if (virFirewallSetBackend(VIR_FIREWALL_BACKEND_DIRECT) < 0) { - return EXIT_FAILURE; - } - basefile = g_strdup_printf("%s/networkxml2firewalldata/base.args", abs_srcdir); if (virFileReadAll(basefile, INT_MAX, &baseargs) < 0) diff --git a/tests/nwfilterebiptablestest.c b/tests/nwfilterebiptablestest.c index 9307a10229..35c1c772ae 100644 --- a/tests/nwfilterebiptablestest.c +++ b/tests/nwfilterebiptablestest.c @@ -26,9 +26,6 @@ #include "virbuffer.h" #include "virfirewall.h" -#define LIBVIRT_VIRFIREWALLPRIV_H_ALLOW -#include "virfirewallpriv.h" - #define LIBVIRT_VIRCOMMANDPRIV_H_ALLOW #include "vircommandpriv.h" @@ -460,10 +457,6 @@ mymain(void) { int ret = 0; - if (virFirewallSetBackend(VIR_FIREWALL_BACKEND_DIRECT) < 0) { - return EXIT_FAILURE; - } - if (virTestRun("ebiptablesAllTeardown", testNWFilterEBIPTablesAllTeardown, NULL) < 0) diff --git a/tests/nwfilterxml2firewalltest.c b/tests/nwfilterxml2firewalltest.c index 857214dde5..ec37a4ae11 100644 --- a/tests/nwfilterxml2firewalltest.c +++ b/tests/nwfilterxml2firewalltest.c @@ -26,9 +26,7 @@ # include "testutils.h" # include "nwfilter/nwfilter_ebiptables_driver.h" # include "virbuffer.h" - -# define LIBVIRT_VIRFIREWALLPRIV_H_ALLOW -# include "virfirewallpriv.h" +# include "virfirewall.h" # define LIBVIRT_VIRCOMMANDPRIV_H_ALLOW # include "vircommandpriv.h" @@ -423,10 +421,6 @@ mymain(void) ret = -1; \ } while (0) - if (virFirewallSetBackend(VIR_FIREWALL_BACKEND_DIRECT) < 0) { - return EXIT_FAILURE; - } - DO_TEST("ah"); DO_TEST("ah-ipv6"); DO_TEST("all"); diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index 724d3081f1..8a0ca6be07 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -25,10 +25,10 @@ # include <gio/gio.h> # include "virbuffer.h" +# include "virfirewall.h" + # define LIBVIRT_VIRCOMMANDPRIV_H_ALLOW # include "vircommandpriv.h" -# define LIBVIRT_VIRFIREWALLPRIV_H_ALLOW -# include "virfirewallpriv.h" # define LIBVIRT_VIRFIREWALLDPRIV_H_ALLOW # include "virfirewalldpriv.h" # include "virmock.h" @@ -779,9 +779,6 @@ mymain(void) { int ret = 0; - if (virFirewallSetBackend(VIR_FIREWALL_BACKEND_DIRECT) < 0) - return EXIT_FAILURE; - # define RUN_TEST(name, method) \ do { \ if (virTestRun(name, method, NULL) < 0) \ -- 2.33.1
There is no longer anything to initialize at binary startup time. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virfirewall.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 98d78857df..70092f2ef6 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -82,15 +82,6 @@ struct _virFirewall { static virMutex ruleLock = VIR_MUTEX_INITIALIZER; -static int -virFirewallOnceInit(void) -{ - return 0; -} - -VIR_ONCE_GLOBAL_INIT(virFirewall); - - static virFirewallGroup * virFirewallGroupNew(void) { @@ -110,12 +101,7 @@ virFirewallGroupNew(void) */ virFirewall *virFirewallNew(void) { - virFirewall *firewall; - - if (virFirewallInitialize() < 0) - return NULL; - - firewall = g_new0(virFirewall, 1); + virFirewall *firewall = g_new0(virFirewall, 1); return firewall; } -- 2.33.1
On 12/12/21 20:48, Laine Stump wrote:
These patches make no functional change, they just remove a bunch of cruft that accumulated over the years and is no longer needed.
This is all in advance of adding support for native nftable support, but there is nothing nftables-specific being added here; I just wanted to get these cleanups out of way now so that the eventual nftables support patchset is smaller and less complicated.
(NB: the concept of a "firewall backend" is being removed here, implying that it will no longer exist. This is not true, but the way that it will exist in the future will be different (per-firewall object rather than per-process) so almost all of the existing code won't be applicable anyway.)
Laine Stump (12): network: eliminate code that uses default iptables chains util: rename/move iptablesFormatNetwork to virSocketAddrFormatWithPrefix util: rename iptables operators to something less generic tests: remove firewalld backend tests from virfirewalltest.c tests: remove unnecessary ret variables and cleanup labels tests: document why virgdbus must be mocked in networkxml2firewalltest.c util: eliminate pointless switch in virFirewallApplyRule util: simplify virFirewallBackendSynchronize() util: move and rename virFirewallBackendSynchronize() util: remove check for iptables binary during virFirewallInit util: remove currentBackend from virfirewall.c util: remove virFirewallOnceInit()
src/libvirt_private.syms | 5 +- src/network/bridge_driver_linux.c | 37 +-- src/util/virfirewall.c | 143 +---------- src/util/virfirewall.h | 2 - src/util/virfirewalld.c | 43 ++++ src/util/virfirewalld.h | 2 + src/util/virfirewallpriv.h | 37 --- src/util/viriptables.c | 207 +++++++--------- src/util/viriptables.h | 2 - src/util/virsocketaddr.c | 44 ++++ src/util/virsocketaddr.h | 3 + tests/networkxml2firewalltest.c | 14 +- tests/nwfilterebiptablestest.c | 7 - tests/nwfilterxml2firewalltest.c | 8 +- tests/virfirewalltest.c | 390 ++++-------------------------- 15 files changed, 247 insertions(+), 697 deletions(-) delete mode 100644 src/util/virfirewallpriv.h
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Laine Stump -
Michal Prívozník