Re: [libvirt] [PATCH v2] configure: Remove build time checks for (ip|ip6|eb)tables
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.
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"?
- 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].
[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?
--
Andrea Bolognani
Software Engineer - Virtualization Team