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@xxxxxxxxxx> >> --- >> >> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
