[libvirt] [PATCH] configure: Remove build time checks for (ip|ip6|eb)tables
Cole Robinson
crobinso at redhat.com
Sat Apr 23 19:40:51 UTC 2016
On 04/22/2016 10:14 AM, Laine Stump wrote:
> 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)
>
I didn't do the spec change... it looks like the test suite is indirectly
dependent on host iptables/ip6tables/ebtables being available, it's calling
into virFirewallValidateBackend somehow. I didn't dig into it though
- Cole
More information about the libvir-list
mailing list