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

[John Ferlan]

On 09/19/2017 03:58 AM, Andrea Bolognani wrote:
> 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

Oh my something strange has happened to your email client with line
wraps on sent mail...

> 
> 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.

I checked - up through 4.8 "most" are available. At 4.8 the *ESWITCH*
ones were added. Checking for a minimum version for a symbol is
something we do in other code under the assumption that any symbol that
is in a counted enum/list for a command or structure before it is
available. Otherwise, we'd have more symbol and field checking all over
the place.

> 
> 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;

That's another option to what I posted a few moments ago to use:

# ifndef...


> 
>   - 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.
> 

I see you've posted and Jano has already responded.  Thanks for the
assistance on this though. Somehow I knew some strange issue would crop
up on some build platform...

John