There are some quirks to detecting whether devlink support can be
activated due to symbols being renamed between Linux versions.

Make detection more robust so that the code can once again compile
on RHEL 7 and others.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 configure.ac         | 26 ++++++++++++++++++++++----
 src/util/virnetdev.c |  5 +++++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac
index c9509c7f9..6f03152b4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -632,12 +632,30 @@ dnl check for kernel headers required by devlink
 dnl
 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_DEFINE([HAVE_DECL_DEVLINK],
-                              [1],
-                              [whether devlink declarations are available])],
+
+    dnl DEVLINK_CMD_ESWITCH_MODE_GET was introduced in Linux 4.8, but Linux
+    dnl 4.11 renamed it to DEVLINK_CMD_ESWITCH_GET. We can use either, but
+    dnl we need at least one of them to be available
+    have_eswitch_get=0
+    AC_CHECK_DECLS([DEVLINK_CMD_ESWITCH_GET, DEVLINK_CMD_ESWITCH_MODE_GET],
+                   [have_eswitch_get=1],
+                   [],
+                   [[#include <linux/devlink.h>]])
+
+    dnl We use a bunch of other symbols as well
+    have_all_others=1
+    AC_CHECK_DECLS([DEVLINK_GENL_VERSION, DEVLINK_GENL_NAME, DEVLINK_ATTR_MAX, DEVLINK_ATTR_BUS_NAME, DEVLINK_ATTR_DEV_NAME, DEVLINK_ATTR_ESWITCH_MODE, DEVLINK_ESWITCH_MODE_SWITCHDEV],
                    [],
+                   [have_all_others=0],
                    [[#include <linux/devlink.h>]])
+
+    dnl If we have at least one variation of DEVLINK_CMD_ESWITCH_GET *and*
+    dnl all other symbols, then we can enable the devlink code
+    if test have_eswitch_get = 1 && test have_all_others = 1; then
+        AC_DEFINE_UNQUOTED([HAVE_DECL_DEVLINK],
+                           [1],
+                           [whether devlink declarations are available])
+    fi
 fi
 
 dnl Allow perl/python overrides
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 040693925..1e0a257e1 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -3245,7 +3245,12 @@ virNetDevSwitchdevFeature(const char *ifname,
     if (!(gmsgh = virNetDevPutExtraHeader(nlmsg_hdr(nl_msg), sizeof(struct genlmsghdr))))
         goto cleanup;
 
+#if HAVE_DEVLINK_CMD_ESWITCH_GET
     gmsgh->cmd = DEVLINK_CMD_ESWITCH_GET;
+#elif HAVE_DEVLINK_CMD_ESWITCH_MODE_GET
+    gmsgh->cmd = DEVLINK_CMD_ESWITCH_MODE_GET;
+#endif
+
     gmsgh->version = DEVLINK_GENL_VERSION;
 
     pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) :
-- 
2.13.5

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

On Tue, Sep 19, 2017 at 10:31:31AM +0200, Andrea Bolognani wrote:
>There are some quirks to detecting whether devlink support can be
>activated due to symbols being renamed between Linux versions.
>
>Make detection more robust so that the code can once again compile
>on RHEL 7 and others.
>
>Signed-off-by: Andrea Bolognani <abologna@redhat.com>
>---
> configure.ac         | 26 ++++++++++++++++++++++----
> src/util/virnetdev.c |  5 +++++
> 2 files changed, 27 insertions(+), 4 deletions(-)
>
>diff --git a/configure.ac b/configure.ac
>index c9509c7f9..6f03152b4 100644
>--- a/configure.ac
>+++ b/configure.ac
>@@ -632,12 +632,30 @@ dnl check for kernel headers required by devlink
> dnl
> 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_DEFINE([HAVE_DECL_DEVLINK],
>-                              [1],
>-                              [whether devlink declarations are available])],
>+
>+    dnl DEVLINK_CMD_ESWITCH_MODE_GET was introduced in Linux 4.8, but Linux
>+    dnl 4.11 renamed it to DEVLINK_CMD_ESWITCH_GET. We can use either, but
>+    dnl we need at least one of them to be available
>+    have_eswitch_get=0

We should not be using deprecated constant names in new code.

>+    AC_CHECK_DECLS([DEVLINK_CMD_ESWITCH_GET, DEVLINK_CMD_ESWITCH_MODE_GET],
>+                   [have_eswitch_get=1],
>+                   [],
>+                   [[#include <linux/devlink.h>]])
>+
>+    dnl We use a bunch of other symbols as well
>+    have_all_others=1

>+    AC_CHECK_DECLS([DEVLINK_GENL_VERSION, DEVLINK_GENL_NAME, DEVLINK_ATTR_MAX, DEVLINK_ATTR_BUS_NAME, DEVLINK_ATTR_DEV_NAME,

All these constants were introduced by commit bfcd3a4661 along with the
devlink infractructure. Having any of them is a sufficient witness for
HAVE_DECL_DEVLINK and pointless to check here.

As for these two:
DEVLINK_ATTR_ESWITCH_MODE, DEVLINK_ESWITCH_MODE_SWITCHDEV],

we only need to check for ESWITCH_MODE. If there are any modes to
check for, SWITCHDEV should be one of the known ones.

>                    [],
>+                   [have_all_others=0],
>                    [[#include <linux/devlink.h>]])
>+
>+    dnl If we have at least one variation of DEVLINK_CMD_ESWITCH_GET *and*
>+    dnl all other symbols, then we can enable the devlink code
>+    if test have_eswitch_get = 1 && test have_all_others = 1; then
>+        AC_DEFINE_UNQUOTED([HAVE_DECL_DEVLINK],
>+                           [1],
>+                           [whether devlink declarations are available])
>+    fi
> fi
>
> dnl Allow perl/python overrides
>diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>index 040693925..1e0a257e1 100644
>--- a/src/util/virnetdev.c
>+++ b/src/util/virnetdev.c
>@@ -3245,7 +3245,12 @@ virNetDevSwitchdevFeature(const char *ifname,
>     if (!(gmsgh = virNetDevPutExtraHeader(nlmsg_hdr(nl_msg), sizeof(struct genlmsghdr))))
>         goto cleanup;
>
>+#if HAVE_DEVLINK_CMD_ESWITCH_GET
>     gmsgh->cmd = DEVLINK_CMD_ESWITCH_GET;
>+#elif HAVE_DEVLINK_CMD_ESWITCH_MODE_GET
>+    gmsgh->cmd = DEVLINK_CMD_ESWITCH_MODE_GET;

We really don't need to litter the code with obsolete constants.

Jan

>+#endif
>+
>     gmsgh->version = DEVLINK_GENL_VERSION;
>
>     pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) :
>-- 
>2.13.5
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, 2017-09-19 at 12:29 +0200, Ján Tomko wrote:
> > +    AC_CHECK_DECLS([DEVLINK_GENL_VERSION, DEVLINK_GENL_NAME, DEVLINK_ATTR_MAX, DEVLINK_ATTR_BUS_NAME, DEVLINK_ATTR_DEV_NAME,
> 
> All these constants were introduced by commit bfcd3a4661 along with the
> devlink infractructure. Having any of them is a sufficient witness for
> HAVE_DECL_DEVLINK and pointless to check here.

I did not dig into the Linux code, so I was not aware of that, but
since that's the case we can definitely reduce the list to a single
item.

> As for these two:
> DEVLINK_ATTR_ESWITCH_MODE, DEVLINK_ESWITCH_MODE_SWITCHDEV],
> 
> we only need to check for ESWITCH_MODE. If there are any modes to
> check for, SWITCHDEV should be one of the known ones.

Wouldn't it make more sense to check for _MODE_SWITCHDEV instead?
Surely if that is available, then _MODE will be too.

> > @@ -3245,7 +3245,12 @@ virNetDevSwitchdevFeature(const char *ifname,
> >     if (!(gmsgh = virNetDevPutExtraHeader(nlmsg_hdr(nl_msg), sizeof(struct genlmsghdr))))
> >         goto cleanup;
> > 
> > +#if HAVE_DEVLINK_CMD_ESWITCH_GET
> >     gmsgh->cmd = DEVLINK_CMD_ESWITCH_GET;
> > +#elif HAVE_DEVLINK_CMD_ESWITCH_MODE_GET
> > +    gmsgh->cmd = DEVLINK_CMD_ESWITCH_MODE_GET;
> 
> We really don't need to litter the code with obsolete constants.

I would totally agree if we were trying to get around the name
change by using the legacy name unconditionally, but my
implementation ensures we're only using the legacy name when the
new one is not available.

So in no situation we're using deprecated symbols, and we can
still compile the devlink code on systems that don't have the
new one such as RHEL 7. Isn't that worth the bit of uglyness the
code above introduces?

-- 
Andrea Bolognani / Red Hat / Virtualization

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

On Tue, Sep 19, 2017 at 01:43:24PM +0200, Andrea Bolognani wrote:
>On Tue, 2017-09-19 at 12:29 +0200, Ján Tomko wrote:
>> > +    AC_CHECK_DECLS([DEVLINK_GENL_VERSION, DEVLINK_GENL_NAME, DEVLINK_ATTR_MAX, DEVLINK_ATTR_BUS_NAME, DEVLINK_ATTR_DEV_NAME,
>>
>> All these constants were introduced by commit bfcd3a4661 along with the
>> devlink infractructure. Having any of them is a sufficient witness for
>> HAVE_DECL_DEVLINK and pointless to check here.
>
>I did not dig into the Linux code, so I was not aware of that, but
>since that's the case we can definitely reduce the list to a single
>item.
>
>> As for these two:
>> DEVLINK_ATTR_ESWITCH_MODE, DEVLINK_ESWITCH_MODE_SWITCHDEV],
>>
>> we only need to check for ESWITCH_MODE. If there are any modes to
>> check for, SWITCHDEV should be one of the known ones.
>
>Wouldn't it make more sense to check for _MODE_SWITCHDEV instead?
>Surely if that is available, then _MODE will be too.
>

_MODE_SWITCHDEV was introduced by commit 08f4b5918b2d along with the
older spelling DEVLINK_CMD_ESWITCH_MODE_GET.

So any release with ESWITCH_MODE does have MODE_SWITCHDEV (until they
feel the need to rename that one too).

>> > @@ -3245,7 +3245,12 @@ virNetDevSwitchdevFeature(const char *ifname,
>> >     if (!(gmsgh = virNetDevPutExtraHeader(nlmsg_hdr(nl_msg), sizeof(struct genlmsghdr))))
>> >         goto cleanup;
>> >
>> > +#if HAVE_DEVLINK_CMD_ESWITCH_GET
>> >     gmsgh->cmd = DEVLINK_CMD_ESWITCH_GET;
>> > +#elif HAVE_DEVLINK_CMD_ESWITCH_MODE_GET
>> > +    gmsgh->cmd = DEVLINK_CMD_ESWITCH_MODE_GET;
>>
>> We really don't need to litter the code with obsolete constants.
>
>I would totally agree if we were trying to get around the name
>change by using the legacy name unconditionally, but my
>implementation ensures we're only using the legacy name when the
>new one is not available.
>
>So in no situation we're using deprecated symbols, and we can
>still compile the devlink code on systems that don't have the
>new one such as RHEL 7. Isn't that worth the bit of uglyness the
>code above introduces?

The original patch for the feature had the new spelling of the
command, I assume the author did not mind it not working with
older kernels.

As for any downstream distributions, I think it's up to them
to either backport the new name for the constant in the kernel
(which would solve this for all apps) or carry the libvirt
workaround themselves.

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