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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list