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

[Andrea Bolognani]

On Mon, 2017-09-18 at 13:21 -0400, 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(-)
> 
> 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

I was looking into this yesterday, unfortunately I didn't manage
to
get anything worth posting ready before leaving for the day...

Anyway, I don't think your approach will work.

First of all, you're removing a number of checks
on unrelated symbols
that are still used in the code, and if any of those is not present
then we shouldn't compile the relevant bits at all.

Second, we're using
DEVLINK_CMD_ESWITCH_GET in the code, but as you
explain that version is only available in newer kernels.

I think the approach need to be more nuanced:

  - use
DEVLINK_CMD_ESWITCH_GET or DEVLINK_CMD_ESWITCH_MODE_GET,
    depending on which on is available, with the former one being the
    preferred option;

  - if neither
DEVLINK_CMD_ESWITCH_GET nor
    DEVLINK_CMD_ESWITCH_MODE_GET is available, then we should compile
    the stub;

  - if any of the other declarations is missing we
should also
    compile the stub.

I'll polish up my patch and post it later today.

-- 
Andrea Bolognani / Red Hat / Virtualization