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

[Laine Stump]

On 04/22/2016 09:42 AM, Cole Robinson wrote:
> On 04/22/2016 09:18 AM, Andrea Bolognani wrote:
>> On Thu, 2016-04-21 at 17:06 -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
>>>   
>>> https://bugzilla.redhat.com/show_bug.cgi?id=661262
>>> ---
>>>    configure.ac            | 12 ------
>>>    src/util/virfirewall.c  | 18 +++++----
>>>    src/util/virnetdev.c    |  6 +--
>>>    tests/virfirewalltest.c | 98 ++++++++++++++++++++++++-------------------------
>>>    4 files changed, 62 insertions(+), 72 deletions(-)
>> I haven't tried running this so I'm probably missing
>> something, but...
>>
>>> @@ -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) {
>>>                    virReportSystemError(errno,
>>>                                         _("direct firewall backend requested, but %s is not available"),
>>>                                         commands[i]);
>>>                    return -1;
>>>                }
>>> +            VIR_FREE(path);
>>>            }
>>>            VIR_DEBUG("found iptables/ip6tables/ebtables, using direct backend");
>>>        }
>> ... how is this fixing the issue reported above?
>>
> Oh, hmm, maybe it doesn't, sorry. I was misreading; I thought the report was
> 'build libvirtd without iptables, install it later, libvirt won't work'.
>
>> AFAICT you just changed it to perform a filesystem lookup instead
>> of relying on the information obtained at configure time. And you
>> removed the check on the file being executable, which is probably
>> not a good idea?
> Judging from the error message it seems like virFileIsExecutable was just a
> surrogate for access(path, F_OK), but I can re add it. As long as someone at
> least thinks this is a worthwhile patch otherwise

I think it's worthwhile; even though the number of self-builders is 
fairly low, they do take time sorting out on IRC. I agree with Andrea 
that the virFileIsExecutable call should remain in, since you can never 
count on one of these self-built systems to have *anything* setup sanely :-P

Once this change is made, I think we can remove all the "BuildRequires: 
(ebtables/iptables/iptables-ipv6)" from the specfile (as long as there 
is no other odd usage of them in configure.ac)