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

John Ferlan posted 1 patch 56 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20170918172147.19870-1-jferlan@redhat.com
configure.ac | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

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

Posted by John Ferlan 56 weeks ago
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@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
-- 
2.13.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Andrea Bolognani 56 weeks ago
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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by John Ferlan 56 weeks ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Andrea Bolognani 56 weeks ago
On Tue, 2017-09-19 at 07:27 -0400, John Ferlan wrote:
> > 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.

Okay, I see what you were aiminig for now, and I don't disagree
with it; however, the rationale was not explained clearly in the
original commit message.

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

Sure, that would work too, and look a little bit better than
what I proposed.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by John Ferlan 56 weeks ago

On 09/19/2017 08:26 AM, Andrea Bolognani wrote:
> On Tue, 2017-09-19 at 07:27 -0400, John Ferlan wrote:
>>> 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.
> 
> Okay, I see what you were aiminig for now, and I don't disagree
> with it; however, the rationale was not explained clearly in the
> original commit message.
> 

OK - I did have it at one point, but the damn commit message got so long
I just trimmed out that part...

>>> 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...
> 
> Sure, that would work too, and look a little bit better than
> what I proposed.
> 

So is this methodology preferred over what Jano posts?

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Erik Skultety 56 weeks ago
On Mon, Sep 18, 2017 at 01:21:47PM -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@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):

Even with the patch the build still fails on my centos-7 instance.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by John Ferlan 56 weeks ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list