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

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. But we cannot just check for the symbol we most care about due to a complication in a kernel 4.11 change that altered the name of the symbol from DEVLINK_CMD_ESWITCH_MODE_GET to DEVLINK_CMD_ESWITCH_GET in the newer kernel and added a #define for the former name with an ominous comment/caveat that the old format is obsolete should never be used. However, if we only looked for the new symbol, that would leave kernels 4.8 through 4.10 unable to use the command even though it exists under an old name. So, 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. This patch checks for both symbol names in order to allow usage and then adds a check to see if the new name doesn't exist after linux/devlink.h is include, then to use #define of the new name to the old name - essentially the reverse logic that the 4.11 header file has. Note that we do not need to check for other symbols as well since they already existed prior to when the various ESWITCH symbols were added. Other defintions include DEVLINK_GENL_VERSION, DEVLINK_GENL_NAME, DEVLINK_ATTR_MAX, DEVLINK_ATTR_BUS_NAME, and DEVLINK_ATTR_DEV_NAME. The DEVLINK_ATTR_ESWITCH_MODE and DEVLINK_ESWITCH_MODE_SWITCHDEV were added at the same time as DEVLINK_CMD_ESWITCH_MODE_GET, but it's only DEVLINK_CMD_ESWITCH_MODE_GET that changed in 4.11. Signed-off-by: John Ferlan <jferlan@redhat.com> --- v1: https://www.redhat.com/archives/libvir-list/2017-September/msg00543.html Just to make it official - here's what I described in the v1 followup which got the ACK if comment from Andrea: https://www.redhat.com/archives/libvir-list/2017-September/msg00591.html Still, I won't push this yet until it too gets an official blessing. configure.ac | 8 ++++++-- src/util/virnetdev.c | 3 +++ 2 files changed, 9 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 diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 040693925..435ca5562 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -61,6 +61,9 @@ #if HAVE_DECL_DEVLINK # include <linux/devlink.h> +# ifndef DEVLINK_CMD_ESWITCH_GET +# define DEVLINK_CMD_ESWITCH_GET DEVLINK_CMD_ESWITCH_MODE_GET +# endif #endif #ifndef IFNAMSIZ -- 2.13.5

On Tue, 2017-09-19 at 11:15 -0400, John Ferlan wrote:
Just to make it official - here's what I described in the v1 followup which got the ACK if comment from Andrea:
https://www.redhat.com/archives/libvir-list/2017-September/msg00591.html
My "ACK if" was to Jano's patch, which he already pushed a few hours ago. -- Andrea Bolognani / Red Hat / Virtualization

On 09/19/2017 12:05 PM, Andrea Bolognani wrote:
On Tue, 2017-09-19 at 11:15 -0400, John Ferlan wrote:
Just to make it official - here's what I described in the v1 followup which got the ACK if comment from Andrea:
https://www.redhat.com/archives/libvir-list/2017-September/msg00591.html
My "ACK if" was to Jano's patch, which he already pushed a few hours ago.
Oh - I misread - great that's fine. Guess I hadn't updated the repo in a bit.... I'm away from the normal place and things are much slower here... Tks - John

On Tue, Sep 19, 2017 at 11:15:28AM -0400, John Ferlan wrote:
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.
But we cannot just check for the symbol we most care about due to a complication in a kernel 4.11 change that altered the name of the symbol from DEVLINK_CMD_ESWITCH_MODE_GET to DEVLINK_CMD_ESWITCH_GET in the newer kernel and added a #define for the former name with an ominous comment/caveat that the old format is obsolete should never be used. However, if we only looked for the new symbol, that would leave kernels 4.8 through 4.10 unable to use the command even though it exists under an old name.
So, 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. This patch checks for both symbol names in order to allow usage and then adds a check to see if the new name doesn't exist after linux/devlink.h is include, then to use #define of the new name to the old name - essentially the reverse logic that the 4.11 header file has.
Note that we do not need to check for other symbols as well since they already existed prior to when the various ESWITCH symbols were added. Other defintions include DEVLINK_GENL_VERSION, DEVLINK_GENL_NAME, DEVLINK_ATTR_MAX, DEVLINK_ATTR_BUS_NAME, and DEVLINK_ATTR_DEV_NAME. The DEVLINK_ATTR_ESWITCH_MODE and DEVLINK_ESWITCH_MODE_SWITCHDEV were added at the same time as DEVLINK_CMD_ESWITCH_MODE_GET, but it's only DEVLINK_CMD_ESWITCH_MODE_GET that changed in 4.11.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
v1: https://www.redhat.com/archives/libvir-list/2017-September/msg00543.html
Just to make it official - here's what I described in the v1 followup which got the ACK if comment from Andrea:
https://www.redhat.com/archives/libvir-list/2017-September/msg00591.html
Still, I won't push this yet until it too gets an official blessing.
configure.ac | 8 ++++++-- src/util/virnetdev.c | 3 +++ 2 files changed, 9 insertions(+), 2 deletions(-)
This looks nicer than Andrea's version that checked them separately at configure-time. Still, I hope we won't need to push this and whoever needs it on an older system can backport the fix to their kernel headers. Jan
participants (3)
-
Andrea Bolognani
-
John Ferlan
-
Ján Tomko