1:48 p.m.
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
1:48 p.m.
New subject: [libvirt PATCH 01/12] network: eliminate code that uses default iptables chains
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(a)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
1:48 p.m.
New subject: [libvirt PATCH 02/12] util: rename/move iptablesFormatNetwork to virSocketAddrFormatWithPrefix
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(a)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
1:48 p.m.
New subject: [libvirt PATCH 03/12] util: rename iptables operators to something less generic
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(a)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
1:48 p.m.
New subject: [libvirt PATCH 04/12] tests: remove firewalld backend tests from virfirewalltest.c
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(a)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
1:48 p.m.
New subject: [libvirt PATCH 05/12] tests: remove unnecessary ret variables and cleanup labels
Several functions were simplified to remove the only cleanup code at
the cleanup label, making it unnecessary.
Signed-off-by: Laine Stump <laine(a)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
1:48 p.m.
New subject: [libvirt PATCH 06/12] tests: document why virgdbus must be mocked in networkxml2firewalltest.c
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(a)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
1:48 p.m.
New subject: [libvirt PATCH 07/12] util: eliminate pointless switch in virFirewallApplyRule
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(a)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
1:48 p.m.
New subject: [libvirt PATCH 08/12] util: simplify virFirewallBackendSynchronize()
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(a)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
1:48 p.m.
New subject: [libvirt PATCH 09/12] util: move and rename virFirewallBackendSynchronize()
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(a)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
1:48 p.m.
New subject: [libvirt PATCH 10/12] util: remove check for iptables binary during virFirewallInit
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(a)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
1:48 p.m.
New subject: [libvirt PATCH 11/12] util: remove currentBackend from virfirewall.c
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(a)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
1:48 p.m.
New subject: [libvirt PATCH 12/12] util: remove virFirewallOnceInit()
There is no longer anything to initialize at binary startup time.
Signed-off-by: Laine Stump <laine(a)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
3:40 a.m.
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(a)redhat.com>
Michal
1078
days inactive
1079
days old
13 comments
2 participants
participants (2)
-
Laine Stump -
Michal Prívozník