diff mbox

[v2] configure: Remove build time checks for (ip|ip6|eb)tables

Message ID 9f305726023e7e2ca017d664150d01de3c8e05ba.1461440358.git.crobinso@redhat.com
State New
Headers show

Commit Message

Cole Robinson April 23, 2016, 7:39 p.m. UTC 
And the 'ip' tool. There isn't much benefit to checking this at
configure time when we have infrastructure nowadays for looking up
binaries in the PATH
---
v2:
    Keep the virFileIsExecutable check


 configure.ac            | 12 ------
 src/util/virfirewall.c  | 18 +++++----
 src/util/virnetdev.c    |  6 +--
 tests/virfirewalltest.c | 98 ++++++++++++++++++++++++-------------------------
 4 files changed, 62 insertions(+), 72 deletions(-)

-- 
2.7.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Cole Robinson April 27, 2016, 4:03 p.m. UTC | #1 
On 04/26/2016 09:42 AM, Andrea Bolognani wrote:
> On Sat, 2016-04-23 at 15:39 -0400, Cole Robinson wrote:

>> And the 'ip' tool. There isn't much benefit to checking this at

>> configure time when we have infrastructure nowadays for looking up

>> binaries in the PATH

>> ---

>> v2:

>>      Keep the virFileIsExecutable check

>>  

>>  

>>   configure.ac            | 12 ------

>>   src/util/virfirewall.c  | 18 +++++----

>>   src/util/virnetdev.c    |  6 +--

>>   tests/virfirewalltest.c | 98 ++++++++++++++++++++++++-------------------------

>>   4 files changed, 62 insertions(+), 72 deletions(-)

> 

> [...]

> 

>> @@ -182,17 +182,19 @@ virFirewallValidateBackend(virFirewallBackend backend)

>>   

>>       if (backend == VIR_FIREWALL_BACKEND_DIRECT) {

>>           const char *commands[] = {

>> -            IPTABLES_PATH, IP6TABLES_PATH, EBTABLES_PATH

>> +            "iptables", "ip6tables", "ebtables"

>>           };

>>           size_t i;

>>   

>>           for (i = 0; i < ARRAY_CARDINALITY(commands); i++) {

>> -            if (!virFileIsExecutable(commands[i])) {

>> +            char *path = virFindFileInPath(commands[i]);

>> +            if (!path || !virFileIsExecutable(path)) {

>>                   virReportSystemError(errno,

>>                                        _("direct firewall backend requested, but %s is not available"),

>>                                        commands[i]);

> 

> You need to VIR_FREE(path) here as well to avoid leaking memory.

> 


Thanks, fixed locally

>>                   return -1;

>>               }

>> +            VIR_FREE(path);

>>           }

>>           VIR_DEBUG("found iptables/ip6tables/ebtables, using direct backend");

>>       }

> 

> [...]

> 

>> --- a/tests/virfirewalltest.c

>> +++ b/tests/virfirewalltest.c

>> @@ -128,11 +128,11 @@ VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block,

>>   

>>           if (fwBuf) {

>>               if (STREQ(type, "ipv4"))

>> -                virBufferAddLit(fwBuf, IPTABLES_PATH);

>> +                virBufferAddLit(fwBuf, "iptables");

>>               else if (STREQ(type, "ipv4"))

> 

> Unrelated to your changes, but shouldn't the above be "ipv6"?

> 


Nice catch :) Indeed that seems wrong, but doesn't look like the test suite
even hits that code path AFAICT, every bit of data it tests is for iptables only

>> -                virBufferAddLit(fwBuf, IP6TABLES_PATH);

>> +                virBufferAddLit(fwBuf, "ip6tables");

>>               else

>> -                virBufferAddLit(fwBuf, EBTABLES_PATH);

>> +                virBufferAddLit(fwBuf, "ebtables");

>>           }

>>           for (i = 0; i < nargs; i++) {

>>               if (fwBuf) {

> 

> [...]

> 

> This series works fine on my Fedora builder, but breaks 'make

> check' on my Debian builder, because iptables is installed

> in /sbin and the user's $PATH doesn't contain that directory,

> which causes virFirewallValidateBackend() to fail.

> 

> I think the proper solution would be to mock filesystem

> access in the test suite so that *tables commands are always

> found. That way you'd be able to run the test suite even

> when *tables commands are not installed on the systems, which

> seems perfectly reasonable if you only ever intend to use the

> firewalld backend[1].

> 


Hmm, okay. I'll put it on my todo list

Thanks,
Cole

> 

> [1] Of course firewalld itself seems to depend on iptables

>     and least recommend ebtables, but I guess that's an

>     implementation detail and could change in the future /

>     on different platforms?

> -- 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
diff mbox

Patch

diff --git a/configure.ac b/configure.ac
index de5f430..35ae16e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -694,18 +694,6 @@  if test x"$with_rhel5_api" = x"yes"; then
    AC_DEFINE([WITH_RHEL5_API], [1], [whether building for the RHEL-5 API])
 fi
 
-AC_PATH_PROG([IP_PATH], [ip], /sbin/ip, [/usr/sbin:$PATH])
-AC_DEFINE_UNQUOTED([IP_PATH], "$IP_PATH", [path to ip binary])
-
-AC_PATH_PROG([IPTABLES_PATH], [iptables], /sbin/iptables, [/usr/sbin:$PATH])
-AC_DEFINE_UNQUOTED([IPTABLES_PATH], "$IPTABLES_PATH", [path to iptables binary])
-
-AC_PATH_PROG([IP6TABLES_PATH], [ip6tables], /sbin/ip6tables, [/usr/sbin:$PATH])
-AC_DEFINE_UNQUOTED([IP6TABLES_PATH], "$IP6TABLES_PATH", [path to ip6tables binary])
-
-AC_PATH_PROG([EBTABLES_PATH], [ebtables], /sbin/ebtables, [/usr/sbin:$PATH])
-AC_DEFINE_UNQUOTED([EBTABLES_PATH], "$EBTABLES_PATH", [path to ebtables binary])
-
 
 dnl
 dnl Checks for the OpenVZ driver
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index f26fd86..0c8e3bf 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -47,9 +47,9 @@  typedef virFirewallGroup *virFirewallGroupPtr;
 
 VIR_ENUM_DECL(virFirewallLayerCommand)
 VIR_ENUM_IMPL(virFirewallLayerCommand, VIR_FIREWALL_LAYER_LAST,
-              EBTABLES_PATH,
-              IPTABLES_PATH,
-              IP6TABLES_PATH);
+              "ebtables",
+              "iptables",
+              "ip6tables");
 
 VIR_ENUM_DECL(virFirewallLayerFirewallD)
 VIR_ENUM_IMPL(virFirewallLayerFirewallD, VIR_FIREWALL_LAYER_LAST,
@@ -134,13 +134,13 @@  static void
 virFirewallCheckUpdateLocking(void)
 {
     const char *iptablesArgs[] = {
-        IPTABLES_PATH, "-w", "-L", "-n", NULL,
+        "iptables", "-w", "-L", "-n", NULL,
     };
     const char *ip6tablesArgs[] = {
-        IP6TABLES_PATH, "-w", "-L", "-n", NULL,
+        "ip6tables", "-w", "-L", "-n", NULL,
     };
     const char *ebtablesArgs[] = {
-        EBTABLES_PATH, "--concurrent", "-L", NULL,
+        "ebtables", "--concurrent", "-L", NULL,
     };
     if (lockOverride)
         return;
@@ -182,17 +182,19 @@  virFirewallValidateBackend(virFirewallBackend backend)
 
     if (backend == VIR_FIREWALL_BACKEND_DIRECT) {
         const char *commands[] = {
-            IPTABLES_PATH, IP6TABLES_PATH, EBTABLES_PATH
+            "iptables", "ip6tables", "ebtables"
         };
         size_t i;
 
         for (i = 0; i < ARRAY_CARDINALITY(commands); i++) {
-            if (!virFileIsExecutable(commands[i])) {
+            char *path = virFindFileInPath(commands[i]);
+            if (!path || !virFileIsExecutable(path)) {
                 virReportSystemError(errno,
                                      _("direct firewall backend requested, but %s is not available"),
                                      commands[i]);
                 return -1;
             }
+            VIR_FREE(path);
         }
         VIR_DEBUG("found iptables/ip6tables/ebtables, using direct backend");
     }
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index bb17b84..75e45fd 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1469,7 +1469,7 @@  int virNetDevSetIPAddress(const char *ifname,
         virCommandAddArgList(cmd, "broadcast", bcaststr, NULL);
     virCommandAddArg(cmd, "alias");
 # else
-    cmd = virCommandNew(IP_PATH);
+    cmd = virCommandNew("ip");
     virCommandAddArgList(cmd, "addr", "add", NULL);
     virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix);
     if (peerstr)
@@ -1506,7 +1506,7 @@  virNetDevAddRoute(const char *ifname,
         goto cleanup;
     if (!(gatewaystr = virSocketAddrFormat(gateway)))
         goto cleanup;
-    cmd = virCommandNew(IP_PATH);
+    cmd = virCommandNew("ip");
     virCommandAddArgList(cmd, "route", "add", NULL);
     virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix);
     virCommandAddArgList(cmd, "via", gatewaystr, "dev", ifname,
@@ -1544,7 +1544,7 @@  int virNetDevClearIPAddress(const char *ifname,
     virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix);
     virCommandAddArg(cmd, "-alias");
 # else
-    cmd = virCommandNew(IP_PATH);
+    cmd = virCommandNew("ip");
     virCommandAddArgList(cmd, "addr", "del", NULL);
     virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix);
     virCommandAddArgList(cmd, "dev", ifname, NULL);
diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c
index f1f29c6..976e883 100644
--- a/tests/virfirewalltest.c
+++ b/tests/virfirewalltest.c
@@ -128,11 +128,11 @@  VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block,
 
         if (fwBuf) {
             if (STREQ(type, "ipv4"))
-                virBufferAddLit(fwBuf, IPTABLES_PATH);
+                virBufferAddLit(fwBuf, "iptables");
             else if (STREQ(type, "ipv4"))
-                virBufferAddLit(fwBuf, IP6TABLES_PATH);
+                virBufferAddLit(fwBuf, "ip6tables");
             else
-                virBufferAddLit(fwBuf, EBTABLES_PATH);
+                virBufferAddLit(fwBuf, "ebtables");
         }
         for (i = 0; i < nargs; i++) {
             if (fwBuf) {
@@ -204,8 +204,8 @@  testFirewallSingleGroup(const void *opaque)
     int ret = -1;
     const char *actual = NULL;
     const char *expected =
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
-        IPTABLES_PATH " -A INPUT --source-host '!192.168.122.1' --jump REJECT\n";
+        "iptables -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
+        "iptables -A INPUT --source-host '!192.168.122.1' --jump REJECT\n";
     const struct testFirewallData *data = opaque;
 
     fwDisabled = data->fwDisabled;
@@ -263,8 +263,8 @@  testFirewallRemoveRule(const void *opaque)
     int ret = -1;
     const char *actual = NULL;
     const char *expected =
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
-        IPTABLES_PATH " -A INPUT --source-host '!192.168.122.1' --jump REJECT\n";
+        "iptables -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
+        "iptables -A INPUT --source-host '!192.168.122.1' --jump REJECT\n";
     const struct testFirewallData *data = opaque;
     virFirewallRulePtr fwrule;
 
@@ -329,10 +329,10 @@  testFirewallManyGroups(const void *opaque ATTRIBUTE_UNUSED)
     int ret = -1;
     const char *actual = NULL;
     const char *expected =
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
-        IPTABLES_PATH " -A INPUT --source-host '!192.168.122.1' --jump REJECT\n"
-        IPTABLES_PATH " -A OUTPUT --source-host 192.168.122.1 --jump ACCEPT\n"
-        IPTABLES_PATH " -A OUTPUT --jump DROP\n";
+        "iptables -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
+        "iptables -A INPUT --source-host '!192.168.122.1' --jump REJECT\n"
+        "iptables -A OUTPUT --source-host 192.168.122.1 --jump ACCEPT\n"
+        "iptables -A OUTPUT --jump DROP\n";
     const struct testFirewallData *data = opaque;
 
     fwDisabled = data->fwDisabled;
@@ -423,10 +423,10 @@  testFirewallIgnoreFailGroup(const void *opaque ATTRIBUTE_UNUSED)
     int ret = -1;
     const char *actual = NULL;
     const char *expected =
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.255 --jump REJECT\n"
-        IPTABLES_PATH " -A OUTPUT --source-host 192.168.122.1 --jump ACCEPT\n"
-        IPTABLES_PATH " -A OUTPUT --jump DROP\n";
+        "iptables -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
+        "iptables -A INPUT --source-host 192.168.122.255 --jump REJECT\n"
+        "iptables -A OUTPUT --source-host 192.168.122.1 --jump ACCEPT\n"
+        "iptables -A OUTPUT --jump DROP\n";
     const struct testFirewallData *data = opaque;
 
     fwDisabled = data->fwDisabled;
@@ -498,10 +498,10 @@  testFirewallIgnoreFailRule(const void *opaque ATTRIBUTE_UNUSED)
     int ret = -1;
     const char *actual = NULL;
     const char *expected =
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.255 --jump REJECT\n"
-        IPTABLES_PATH " -A OUTPUT --source-host 192.168.122.1 --jump ACCEPT\n"
-        IPTABLES_PATH " -A OUTPUT --jump DROP\n";
+        "iptables -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
+        "iptables -A INPUT --source-host 192.168.122.255 --jump REJECT\n"
+        "iptables -A OUTPUT --source-host 192.168.122.1 --jump ACCEPT\n"
+        "iptables -A OUTPUT --jump DROP\n";
     const struct testFirewallData *data = opaque;
 
     fwDisabled = data->fwDisabled;
@@ -572,8 +572,8 @@  testFirewallNoRollback(const void *opaque ATTRIBUTE_UNUSED)
     int ret = -1;
     const char *actual = NULL;
     const char *expected =
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.255 --jump REJECT\n";
+        "iptables -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
+        "iptables -A INPUT --source-host 192.168.122.255 --jump REJECT\n";
     const struct testFirewallData *data = opaque;
 
     fwDisabled = data->fwDisabled;
@@ -642,11 +642,11 @@  testFirewallSingleRollback(const void *opaque ATTRIBUTE_UNUSED)
     int ret = -1;
     const char *actual = NULL;
     const char *expected =
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.255 --jump REJECT\n"
-        IPTABLES_PATH " -D INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
-        IPTABLES_PATH " -D INPUT --source-host 192.168.122.255 --jump REJECT\n"
-        IPTABLES_PATH " -D INPUT --source-host '!192.168.122.1' --jump REJECT\n";
+        "iptables -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
+        "iptables -A INPUT --source-host 192.168.122.255 --jump REJECT\n"
+        "iptables -D INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
+        "iptables -D INPUT --source-host 192.168.122.255 --jump REJECT\n"
+        "iptables -D INPUT --source-host '!192.168.122.1' --jump REJECT\n";
     const struct testFirewallData *data = opaque;
 
     fwDisabled = data->fwDisabled;
@@ -732,10 +732,10 @@  testFirewallManyRollback(const void *opaque ATTRIBUTE_UNUSED)
     int ret = -1;
     const char *actual = NULL;
     const char *expected =
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.255 --jump REJECT\n"
-        IPTABLES_PATH " -D INPUT --source-host 192.168.122.255 --jump REJECT\n"
-        IPTABLES_PATH " -D INPUT --source-host '!192.168.122.1' --jump REJECT\n";
+        "iptables -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
+        "iptables -A INPUT --source-host 192.168.122.255 --jump REJECT\n"
+        "iptables -D INPUT --source-host 192.168.122.255 --jump REJECT\n"
+        "iptables -D INPUT --source-host '!192.168.122.1' --jump REJECT\n";
     const struct testFirewallData *data = opaque;
 
     fwDisabled = data->fwDisabled;
@@ -825,14 +825,14 @@  testFirewallChainedRollback(const void *opaque ATTRIBUTE_UNUSED)
     int ret = -1;
     const char *actual = NULL;
     const char *expected =
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.127 --jump REJECT\n"
-        IPTABLES_PATH " -A INPUT --source-host '!192.168.122.1' --jump REJECT\n"
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.255 --jump REJECT\n"
-        IPTABLES_PATH " -D INPUT --source-host 192.168.122.127 --jump REJECT\n"
-        IPTABLES_PATH " -D INPUT --source-host '!192.168.122.1' --jump REJECT\n"
-        IPTABLES_PATH " -D INPUT --source-host 192.168.122.255 --jump REJECT\n"
-        IPTABLES_PATH " -D INPUT --source-host '!192.168.122.1' --jump REJECT\n";
+        "iptables -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
+        "iptables -A INPUT --source-host 192.168.122.127 --jump REJECT\n"
+        "iptables -A INPUT --source-host '!192.168.122.1' --jump REJECT\n"
+        "iptables -A INPUT --source-host 192.168.122.255 --jump REJECT\n"
+        "iptables -D INPUT --source-host 192.168.122.127 --jump REJECT\n"
+        "iptables -D INPUT --source-host '!192.168.122.1' --jump REJECT\n"
+        "iptables -D INPUT --source-host 192.168.122.255 --jump REJECT\n"
+        "iptables -D INPUT --source-host '!192.168.122.1' --jump REJECT\n";
     const struct testFirewallData *data = opaque;
 
     fwDisabled = data->fwDisabled;
@@ -976,11 +976,11 @@  testFirewallQueryHook(const char *const*args,
                       int *status,
                       void *opaque ATTRIBUTE_UNUSED)
 {
-    if (STREQ(args[0], IPTABLES_PATH) &&
+    if (STREQ(args[0], "iptables") &&
         STREQ(args[1], "-L")) {
         if (VIR_STRDUP(*output, TEST_FILTER_TABLE_LIST) < 0)
             *status = 127;
-    } else if (STREQ(args[0], IPTABLES_PATH) &&
+    } else if (STREQ(args[0], "iptables") &&
                STREQ(args[1], "-t") &&
                STREQ(args[2], "nat") &&
                STREQ(args[3], "-L")) {
@@ -1026,15 +1026,15 @@  testFirewallQuery(const void *opaque ATTRIBUTE_UNUSED)
     int ret = -1;
     const char *actual = NULL;
     const char *expected =
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.127 --jump REJECT\n"
-        IPTABLES_PATH " -L\n"
-        IPTABLES_PATH " -t nat -L\n"
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.130 --jump REJECT\n"
-        IPTABLES_PATH " -A INPUT --source-host '!192.168.122.129' --jump REJECT\n"
-        IPTABLES_PATH " -A INPUT --source-host '!192.168.122.129' --jump REJECT\n"
-        IPTABLES_PATH " -A INPUT --source-host 192.168.122.128 --jump REJECT\n"
-        IPTABLES_PATH " -A INPUT --source-host '!192.168.122.1' --jump REJECT\n";
+        "iptables -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
+        "iptables -A INPUT --source-host 192.168.122.127 --jump REJECT\n"
+        "iptables -L\n"
+        "iptables -t nat -L\n"
+        "iptables -A INPUT --source-host 192.168.122.130 --jump REJECT\n"
+        "iptables -A INPUT --source-host '!192.168.122.129' --jump REJECT\n"
+        "iptables -A INPUT --source-host '!192.168.122.129' --jump REJECT\n"
+        "iptables -A INPUT --source-host 192.168.122.128 --jump REJECT\n"
+        "iptables -A INPUT --source-host '!192.168.122.1' --jump REJECT\n";
     const struct testFirewallData *data = opaque;
 
     expectedLineNum = 0;