util/tests: enable locking on iptables/ebtables commandlines by default

iptables and ip6tables have had a "-w" commandline option to grab a
systemwide lock that prevents two iptables invocations from modifying
the iptables chains since 2013 (upstream commit 93587a04 in
iptables-1.4.20).  Similarly, ebtables has had a "--concurrent"
commandline option for the same purpose since 2011 (in the upstream
ebtables commit f9b4bcb93, which was present in ebtables-2.0.10.4).

Libvirt added code to conditionally use the commandline option for
iptables/ip6tables in upstream commit ba95426d6f (libvirt-1.2.0,
November 2013), and for ebtables in upstream commit dc33e6e4a5
(libvirt-1.2.11, November 2014) (the latter actually *re*-added the
locking for iptables/ip6tables, as it had accidentally been removed
during a refactor of firewall code in the interim).

I say "conditionally" because a check was made during firewall module
initialization that tried executing a test command with the
-w/--concurrent option, and only continued using it for actual
commands if that test command completed successfully. At the time the
code was added this was a reasonable thing to do, as it had been less
than a year since introduction of -w to iptables, so many distros
supported by libvirt were still using iptables (and possibly even
ebtables) versions too old to have the new commandline options.

It is now 2020, and as far as I can discern from repology.org (and
manually examining a RHEL7.9 system), every version of every distro
that is supported by libvirt now uses new enough versions of both
iptables and ebtables that they all have support for -w/--concurrent.
That means we can finally remove the conditional code and simply
always use them.

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f5a8209efd011b68bd4d7d19404ddb080d68be85..95bdc6e9c1fd064b5cebb9c579e690f7c990491c 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2158,7 +2158,6 @@ virFirewallRuleAddArgList;
 virFirewallRuleAddArgSet;
 virFirewallRuleGetArgCount;
 virFirewallSetBackend;
-virFirewallSetLockOverride;
 virFirewallStartRollback;
 virFirewallStartTransaction;
 

diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index 5f30c344832314133a588d09488b2853d910dd08..694bb32f629112cde85dd337499b9f208f6eb801 100644 (file)
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -96,59 +96,6 @@ virFirewallOnceInit(void)
 
 VIR_ONCE_GLOBAL_INIT(virFirewall);
 
-static bool iptablesUseLock;
-static bool ip6tablesUseLock;
-static bool ebtablesUseLock;
-static bool lockOverride; /* true to avoid lock probes */
-
-void
-virFirewallSetLockOverride(bool avoid)
-{
-    lockOverride = avoid;
-    if (avoid) {
-        /* add the lock option to all commands */
-        iptablesUseLock = true;
-        ip6tablesUseLock = true;
-        ebtablesUseLock = true;
-    }
-}
-
-static void
-virFirewallCheckUpdateLock(bool *lockflag,
-                           const char *const*args)
-{
-    int status; /* Ignore failed commands without logging them */
-    g_autoptr(virCommand) cmd = virCommandNewArgs(args);
-    if (virCommandRun(cmd, &status) < 0 || status) {
-        VIR_INFO("locking not supported by %s", args[0]);
-    } else {
-        VIR_INFO("using locking for %s", args[0]);
-        *lockflag = true;
-    }
-}
-
-static void
-virFirewallCheckUpdateLocking(void)
-{
-    const char *iptablesArgs[] = {
-        IPTABLES_PATH, "-w", "-L", "-n", NULL,
-    };
-    const char *ip6tablesArgs[] = {
-        IP6TABLES_PATH, "-w", "-L", "-n", NULL,
-    };
-    const char *ebtablesArgs[] = {
-        EBTABLES_PATH, "--concurrent", "-L", NULL,
-    };
-    if (lockOverride)
-        return;
-    virFirewallCheckUpdateLock(&iptablesUseLock,
-                               iptablesArgs);
-    virFirewallCheckUpdateLock(&ip6tablesUseLock,
-                               ip6tablesArgs);
-    virFirewallCheckUpdateLock(&ebtablesUseLock,
-                               ebtablesArgs);
-}
-
 static int
 virFirewallValidateBackend(virFirewallBackend backend)
 {
@@ -196,8 +143,6 @@ virFirewallValidateBackend(virFirewallBackend backend)
 
     currentBackend = backend;
 
-    virFirewallCheckUpdateLocking();
-
     return 0;
 }
 
@@ -359,16 +304,13 @@ virFirewallAddRuleFullV(virFirewallPtr firewall,
 
     switch (rule->layer) {
     case VIR_FIREWALL_LAYER_ETHERNET:
-        if (ebtablesUseLock)
-            ADD_ARG(rule, "--concurrent");
+        ADD_ARG(rule, "--concurrent");
         break;
     case VIR_FIREWALL_LAYER_IPV4:
-        if (iptablesUseLock)
-            ADD_ARG(rule, "-w");
+        ADD_ARG(rule, "-w");
         break;
     case VIR_FIREWALL_LAYER_IPV6:
-        if (ip6tablesUseLock)
-            ADD_ARG(rule, "-w");
+        ADD_ARG(rule, "-w");
         break;
     case VIR_FIREWALL_LAYER_LAST:
         break;

diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h
index 6148f46827291c351188b65eebcbbd130a83deef..fda3cdec01e6d9b42193767b77abcfd29e824f22 100644 (file)
--- a/src/util/virfirewall.h
+++ b/src/util/virfirewall.h
@@ -111,6 +111,4 @@ void virFirewallStartRollback(virFirewallPtr firewall,
 
 int virFirewallApply(virFirewallPtr firewall);
 
-void virFirewallSetLockOverride(bool avoid);
-
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virFirewall, virFirewallFree);

diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c
index 3496445f0dc780ccf1df5133c885659dbafb8b2e..d358f12897b7e46b8c37dee0feb6a81b20ab12a2 100644 (file)
--- a/tests/networkxml2firewalltest.c
+++ b/tests/networkxml2firewalltest.c
@@ -179,8 +179,6 @@ mymain(void)
             ret = -1; \
     } while (0)
 
-    virFirewallSetLockOverride(true);
-
     if (virFirewallSetBackend(VIR_FIREWALL_BACKEND_DIRECT) < 0) {
         if (!hasNetfilterTools()) {
             fprintf(stderr, "iptables/ip6tables/ebtables tools not present");

diff --git a/tests/nwfilterebiptablestest.c b/tests/nwfilterebiptablestest.c
index 5562682e9af08d95f84399ea19647d9ea8916b65..f47b4f1dfd40a31ee12d9c55a845fb9bd516992f 100644 (file)
--- a/tests/nwfilterebiptablestest.c
+++ b/tests/nwfilterebiptablestest.c
@@ -503,8 +503,6 @@ mymain(void)
 {
     int ret = 0;
 
-    virFirewallSetLockOverride(true);
-
     if (virFirewallSetBackend(VIR_FIREWALL_BACKEND_DIRECT) < 0) {
         if (!hasNetfilterTools()) {
             fprintf(stderr, "iptables/ip6tables/ebtables tools not present");

diff --git a/tests/nwfilterxml2firewalltest.c b/tests/nwfilterxml2firewalltest.c
index 7427108e3979c633523162577245b0962eecec53..0901250aafd36114956ab60683a51e5e3cedea74 100644 (file)
--- a/tests/nwfilterxml2firewalltest.c
+++ b/tests/nwfilterxml2firewalltest.c
@@ -457,8 +457,6 @@ mymain(void)
             ret = -1; \
     } while (0)
 
-    virFirewallSetLockOverride(true);
-
     if (virFirewallSetBackend(VIR_FIREWALL_BACKEND_DIRECT) < 0) {
         if (!hasNetfilterTools()) {
             fprintf(stderr, "iptables/ip6tables/ebtables tools not present");

diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c
index bf1d3250176e1b1cedbffd8c07fdf6667f404024..fac7e20c0652f2ec1a6ed4d6265c871c554d7dfe 100644 (file)
--- a/tests/virfirewalltest.c
+++ b/tests/virfirewalltest.c
@@ -1076,8 +1076,6 @@ mymain(void)
     RUN_TEST_DIRECT(name, method); \
     RUN_TEST_FIREWALLD(name, method)
 
-    virFirewallSetLockOverride(true);
-
     RUN_TEST("single group", testFirewallSingleGroup);
     RUN_TEST("remove rule", testFirewallRemoveRule);
     RUN_TEST("many groups", testFirewallManyGroups);