On 04/07/2015 11:47 PM, Eric Blake wrote:
On 04/07/2015 04:20 PM, Laine Stump wrote:
> When netcf first include support for libnl3, the author of that patch
> (Serge Hallyn, commit 0310cd505) found that
> /usr/include/libnl3/netlink/route/link.h #included <linux/if.h>, which
> conflicts with <net/if.h> (but for our purposes at least defines all the
> same things). So he put the #include <net/if.h> in dutil_linux.c inside
> an #ifndef HAVE_LIBNL3.
>
> That worked fine until libnl3 finally removed the offending (and
> unnecessary) #include <linux/if.h> from its link.h. That fix to libnl3
> broke the netcf build.
>
> This patch fixes the build by using autotools magic to attempt
> compiling essentially this short program:
>
> #include <netlink/route/link.h>
> int main()
> {
> int try = IFF_UP;
> struct ifreq ifr;
> return 0;
If someone configures with high levels of warnings enabled, this might
spuriously fail due to unused variables. Something like:
struct ifreq ifr;
return ifr.ifr_ivalue = IFF_UP;
is slightly stronger (nothing goes unused), even if weird (no one would
actually write this anywhere besides configure.ac). Then again, the
autoconf list frequently tells people that run './configure
CFLAGS=-Werror' to not do it - there is NO way to write warning-free
code for every possible compiler version, especially when probing for
features; and therefore worrying about compiler warnings in configure
snippets is not worth the effort. So I would not bother rewriting the
code _unless_ someone complains that it has a false failure during
configure, which I doubt will happen.
Still, I like the idea of creating less warnings to confuse the
config.log output. I'll change it as you suggest before pushing.
> }
>
> If this can compile successfully without any other includes, then
> link.h is already including linux/if.h and we should avoid including
> net/if.h, so we #define AVOID_NET_IF_H. If the compile fails, then we
> do need to include net/if.h, so we *don't* #define
> AVOID_NET_IF_H. Then we enclose dutil_linux.c's #include <net/if.h>
> within #ifndef AVOID_NET_IF_H and we're done.
>
> Although this check would probably be safe on a libnl1 system, we know
> from past experience that we should always include net/if.h in those
> cases anyway, so we don't even try - we only do the configure-time
> check if we're using libnl3.
> ---
> configure.ac | 19 ++++++++++++++++++-
> src/dutil_linux.c | 4 ++--
> 2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index a914b0f..6edbb5e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -164,7 +164,24 @@ then
> if (test "${have_libnl3}" = "yes" -a
"${have_libnl_route3}" = "yes"); then
> AC_DEFINE([HAVE_LIBNL3], [1], [Use libnl-3.0])
> have_libnl="yes"
> - else
> + SAVE_CFLAGS="${CFLAGS}"
Difference in TAB vs. space (not fatal, but you may want to be consistent)
My emacs is setup to pretty much always insert spaces instead of tabs.
It looks like a few committers in the past have used tabs, though. Since
the file is already mixed (mostly spaces, though) and I don't want to
create a bunch of merge conflict opportunities, I'm going to leave my
additions using spaces.
> + CFLAGS="${LIBNL_CFLAGS} ${CFLAGS}"
> + dnl Avoid #including net/if.h when using earlier versions
> + dnl of libnl3 that include linux/if.h from netlink/route/link.h,
> + dnl since the two if.h files conflict with each other.
> + AC_COMPILE_IFELSE([AC_LANG_PROGRAM(
> + [[
> + #include <netlink/route/link.h>
> + ]],
> + [[struct ifreq ifr; int try = IFF_UP;]])],
> + [avoid_net_if_h=yes], [avoid_net_if_h=no])
Looks correct. The only improvement might be to wrap it all in
AC_CACHE_CHECK and rename the witness variable to something like
netcf_cv_avoid_net_if_h, so that it can be primed through a configure
cache rather than probed every time. Again, not sure it would matter
until someone actually complains that it guessed wrong and they wished
for a way to prime the cache to avoid the wrong guess.
Since I don't understand AC_CACHE_CHECK and want to get this in fairly
quickly, I think I won't try to add it.
> + CFLAGS="${SAVE_CFLAGS}"
> + if test "x$avoid_net_if_h" = "xyes"; then
> + AC_DEFINE([AVOID_NET_IF_H], [1],
> + [define to 1 if the libnl3 header netlink/route/link.h
already
> + includes linux/if.h (which will conflict with
net/if.h)])
technically, it conflicts with net/if.h only in some versions of the
kernel headers, but I can't think of any concise wording for that.
It creates a conflict on every version that I've tried, but that sample
is very limited, so I'll take your word for that :-). I'll change it
from "will conflict" to "can conflict".
> + fi
> + else
> PKG_CHECK_MODULES([LIBNL], [libnl-1],
> [have_libnl1=yes],
> [have_libnl1=no])
> diff --git a/src/dutil_linux.c b/src/dutil_linux.c
> index 299b36e..0850593 100644
> --- a/src/dutil_linux.c
> +++ b/src/dutil_linux.c
> @@ -54,8 +54,8 @@
> #include "dutil.h"
> #include "dutil_linux.h"
>
> -#ifndef HAVE_LIBNL3
> -#include <net/if.h>
> +#ifndef AVOID_NET_IF_H
> +# include <net/if.h>
> #endif
> #include <netlink/socket.h>
> #include <netlink/cache.h>
>
ACK.
Thanks!