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

[Andrea Bolognani]

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