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

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)