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? --