[libvirt] [PATCH] util: Fix configure.ac check for DEVLINK_CMD_ESWITCH_GET

[John Ferlan]

On 09/18/2017 01:21 PM, John Ferlan wrote:
> Rather than checking for whether the devlink.h on the system has
> multiple symbols, let's only check for whether the command we want
> is defined.
> 
> Turns out the mechanism of providing multiple definitions to check via
> AC_CHECK_DECLS in order to determine whether HAVE_DECL_DEVLINK should
> be set resulted in a false positive since as long as one of the defs
> was true, then the HAVE_DECL_DEVLINK got defined.
> 
> This is further complicated by a change between kernel 4.8 and 4.11
> where the originally defined name DEVLINK_CMD_ESWITCH_MODE_GET was
> changed to DEVLINK_CMD_ESWITCH_GET with the comment/caveat that
> the old format is obsolete should never be used. Therefore, even
> though some kernels will have a couple of the same symbols that
> were added at the same time (DEVLINK_ATTR_ESWITCH_MODE and
> DEVLINK_ESWITCH_MODE_SWITCHDEV), they won't have the newer name
> and thus cause a build failure.
> 
> So even though multiple DEVLINK_CMD_SWITCH* symbols are used to
> determine whether the set HAVE_DECL_DEVLINK, this should cover
> those kernels before 4.11 with the old definition.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
> 
>  I'd push this as a build breaker, but I don't want to need to go through
>  the trouble again if i got this wrong, so hopefully someone who's seeing
>  this can try out the patch...  It's also present on a couple of the CI
>  infrastructure environments (f23, centos-7):
> 
>   https://ci.centos.org/view/libvirt/job/libvirt-master-build/
> 
>  configure.ac | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 

UGH!  While not a total SNAK, I realized sometime early this morning
(who knows why) - this won't work because it has the same problem as the
current code, but with far less AC_CHECK_DECLS options.

I think though what could be done is add:

# ifndef DEVLINK_CMD_ESWITCH_GET
# define DEVLINK_CMD_ESWITCH_GET DEVLINK_CMD_ESWITCH_MODE_GET
# endif

to src/util/virnetdev.c after it includes <linux/devlink.h>

for some reason I have a recollection we've done something like that
before for a similar issue.  In the end what the kernel does in 4.11 by
adding a #define DEVLINK_CMD_ESWITCH_MODE_GET DEVLINK_CMD_ESWITCH_GET is
the opposite mechanism.

If that doesn't work, then I think just go with DEVLINK_CMD_ESWITCH_GET
in the AC_CHECK_DECLS below and let someone else figure out how to make
it work right!

John


> diff --git a/configure.ac b/configure.ac
> index c9509c7f9..73ae7fdd5 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -630,12 +630,16 @@ fi
>  dnl
>  dnl check for kernel headers required by devlink
>  dnl
> +dnl kernel 4.8 introduced DEVLINK_CMD_ESWITCH_MODE_GET, but that was
> +dnl later changed in kernel 4.11 to be just DEVLINK_CMD_ESWITCH_GET, so
> +dnl for "completeness" let's allow HAVE_DECL_DEVLINK to be define if
> +dnl either is defined.
>  if test "$with_linux" = "yes"; then
>      AC_CHECK_HEADERS([linux/devlink.h])
> -    AC_CHECK_DECLS([DEVLINK_GENL_VERSION, DEVLINK_GENL_NAME, DEVLINK_ATTR_MAX, DEVLINK_CMD_ESWITCH_GET, DEVLINK_ATTR_BUS_NAME, DEVLINK_ATTR_DEV_NAME, DEVLINK_ATTR_ESWITCH_MODE, DEVLINK_ESWITCH_MODE_SWITCHDEV],
> +    AC_CHECK_DECLS([DEVLINK_CMD_ESWITCH_GET, DEVLINK_CMD_ESWITCH_MODE_GET],
>                     [AC_DEFINE([HAVE_DECL_DEVLINK],
>                                [1],
> -                              [whether devlink declarations are available])],
> +                              [whether devlink CMD_ESWITCH_GET is available])],
>                     [],
>                     [[#include <linux/devlink.h>]])
>  fi
>