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