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

[Cole Robinson]

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