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

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