[PATCH] iw: add kernel version checks for pending upstream kernel features

[PATCH] iw: add kernel version checks for pending upstream kernel features

[John W. Linville]

Add checks for kernel version to enable features in the iw tool.  This
allows packagers to get pre-package the tool without requiring precise
timing to stage the tool into their distributions.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 info.c |    4 ++++
 reg.c  |    6 ++++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/info.c b/info.c
index 0dc05c1..4b7d191 100644
--- a/info.c
+++ b/info.c
@@ -1,4 +1,5 @@
 #include <errno.h>
+#include <linux/version.h>
 #include <linux/nl80211.h>
 #include <net/if.h>
 
@@ -105,12 +106,15 @@ static int print_phy_handler(struct nl_msg *msg, void *arg)
 		}
 	}
 
+/* This funcionality first appears "officially" in 2.6.28... */
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,28)
 	if (!tb_msg[NL80211_ATTR_SUPPORTED_IFTYPES])
 		return NL_SKIP;
 
 	printf("\tSupported interface modes:\n");
 	nla_for_each_nested(nl_mode, tb_msg[NL80211_ATTR_SUPPORTED_IFTYPES], rem_mode)
 		printf("\t\t * %s\n", iftype_name(nl_mode->nla_type));
+#endif
 
 	return NL_SKIP;
 }
diff --git a/reg.c b/reg.c
index 2892117..ce039ae 100644
--- a/reg.c
+++ b/reg.c
@@ -1,3 +1,4 @@
+#include <linux/version.h>
 #include <linux/nl80211.h>
 #include <net/if.h>
 #include <errno.h>
@@ -11,6 +12,9 @@
 
 #include "iw.h"
 
+/* This funcionality first appears "officially" in 2.6.28... */
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,28)
+
 int isalpha_upper(char letter)
 {
 	if (letter >= 65 && letter <= 90)
@@ -67,3 +71,5 @@ static int handle_reg_set(struct nl_cb *cb,
 }
 COMMAND(reg, set, "<ISO/IEC 3166-1 alpha2>",
 	NL80211_CMD_REQ_SET_REG, 0, CIB_NONE, handle_reg_set);
+
+#endif /* LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,28) */
-- 
1.5.4.3

Re: [PATCH] iw: add kernel version checks for pending upstream kernel features

[Pavel Roskin]

On Thu, 2008-09-18 at 10:35 -0400, John W. Linville wrote:
> Add checks for kernel version to enable features in the iw tool.  This
> allows packagers to get pre-package the tool without requiring precise
> timing to stage the tool into their distributions.

I'm afraid it's not a good approach.  Suppose a distro takes this
version of iw and Linux 2.6.27.  Later it issues an update for the
kernel that updates it to 2.6.28.  Userspace tools don't normally depend
on the specific version of the kernel, and that's a good thing.  But in
this case, it means that iw won't be recompiled and updated unless the
distro has some mechanism in place to detect such need.  Since the
kernel version detection is done on the compiler level, I suspect such
need won't be detected.

The end result is that iw won't be able to access all functionality of
the kernel.

I believe userspace tools should always do the best effort to support
all functionality they can support, but fail gracefully if any kernel
functionality is not available.

Keeping a local version of nl80211.h in iw sources seems the most
sensible solution to me.  It has its downsides, but the upside is that
there is a clear separation between the userspace and the kernel code.

I know something about Fedora packaging and I assisted developers of
other userspace tools with similar issues.  Of course, opinions of the
actual package maintainers would have more weight.

-- 
Regards,
Pavel Roskin

Re: [PATCH] iw: add kernel version checks for pending upstream kernel features

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]

On Thu, 2008-09-18 at 10:35 -0400, John W. Linville wrote:
>  
> +/* This funcionality first appears "officially" in 2.6.28... */
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,28)

This bites all developers of the tool. I'd use #ifdef
NL80211_ATTR_SUPPORTED_IFTYPES like Jouni did for a bunch of things but
I forgot to add
#define NL80211_ATTR_SUPPORTED_IFTYPES NL80211_ATTR_SUPPORTED_IFTYPES
to nl80211.h

On the other hand, I'm with Pavel in that it sucks if you have a tool
which your distro compiled against 2.6.27 and then suddenly you want to
upgrade your kernel to 2.6.28 and the features don't work...

I don't know. Tell me what to do. I don't really like shipping the
header file either but it may be the lesser of two evils?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] iw: add kernel version checks for pending upstream kernel features

[Pavel Roskin]

On Thu, 2008-09-18 at 17:41 +0200, Johannes Berg wrote:
> On Thu, 2008-09-18 at 10:35 -0400, John W. Linville wrote:
> >  
> > +/* This funcionality first appears "officially" in 2.6.28... */
> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,28)
> 
> This bites all developers of the tool. I'd use #ifdef
> NL80211_ATTR_SUPPORTED_IFTYPES like Jouni did for a bunch of things but
> I forgot to add
> #define NL80211_ATTR_SUPPORTED_IFTYPES NL80211_ATTR_SUPPORTED_IFTYPES
> to nl80211.h

I agree.  I always prefer to test for features, not for numbers.

> On the other hand, I'm with Pavel in that it sucks if you have a tool
> which your distro compiled against 2.6.27 and then suddenly you want to
> upgrade your kernel to 2.6.28 and the features don't work...
> 
> I don't know. Tell me what to do. I don't really like shipping the
> header file either but it may be the lesser of two evils?

I think the build system should use the included header by default.  But
it should be easy to enable compiling against the kernel sources.  For
instance, if the kernel path is defined in .config or there is an option
on the command line.

-- 
Regards,
Pavel Roskin

Re: [PATCH] iw: add kernel version checks for pending upstream kernel features

[Marcel Holtmann]

Hi Johannes,

> > +/* This funcionality first appears "officially" in 2.6.28... */
> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,28)
> 
> This bites all developers of the tool. I'd use #ifdef
> NL80211_ATTR_SUPPORTED_IFTYPES like Jouni did for a bunch of things but
> I forgot to add
> #define NL80211_ATTR_SUPPORTED_IFTYPES NL80211_ATTR_SUPPORTED_IFTYPES
> to nl80211.h
> 
> On the other hand, I'm with Pavel in that it sucks if you have a tool
> which your distro compiled against 2.6.27 and then suddenly you want to
> upgrade your kernel to 2.6.28 and the features don't work...

if the numbers you are planning to use are stable, then please use the
above approach to just add the missing defines to it. Like adding a
compat.h file with

#ifndef NL80211_ATTR_SUPPORTED_IFTYPES
#define NL80211_ATTR_SUPPORTED_IFTYPES boo
#endif

That would make a distro complied binary work with an updates kernel and
we don't end up with the issue the iwconfig current has when it has been
compiled for an older kernel.

However make sure to keep you numbers stable. Otherwise you break kernel
and/or userspace assumptions.

Regards

Marcel

Re: [PATCH] iw: add kernel version checks for pending upstream kernel features

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

Hi Marcel,

On Thu, 2008-09-18 at 19:26 +0200, Marcel Holtmann wrote:

> if the numbers you are planning to use are stable, then please use the
> above approach to just add the missing defines to it. Like adding a
> compat.h file with
> 
> #ifndef NL80211_ATTR_SUPPORTED_IFTYPES
> #define NL80211_ATTR_SUPPORTED_IFTYPES boo
> #endif

Well, at that point I can also just copy nl80211.h from wireless-testing
whenever I think the numbers will remain stable, no? And that doesn't
work as-is anyway since nl80211.h uses an enum to define the numbers,
not #defines.

I guess it's the lesser of all the evils. I'm not too fond of copying in
the header file, it creates more work and doesn't make all that much
sense, but all the other proposals take even more work, and as long as
we don't change the numbers after putting things into wireless-testing
we should be fine.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] iw: add kernel version checks for pending upstream kernel features

[Pavel Roskin]

On Thu, 2008-09-18 at 19:26 +0200, Marcel Holtmann wrote:

> if the numbers you are planning to use are stable, then please use the
> above approach to just add the missing defines to it. Like adding a
> compat.h file with
> 
> #ifndef NL80211_ATTR_SUPPORTED_IFTYPES
> #define NL80211_ATTR_SUPPORTED_IFTYPES boo
> #endif

I'm sure the numbers used to communicate between the kernel and the
userspace are meant to be stable.

> That would make a distro complied binary work with an updates kernel and
> we don't end up with the issue the iwconfig current has when it has been
> compiled for an older kernel.

Having the include file in the sources would have the same effect.

But your approach is easier to get wrong.  Some numbers are from enum,
not from preprocessor defines.  It's easy to put a wrong number into the
compatibility code.  This code will need to be tested on older kernels.

-- 
Regards,
Pavel Roskin

Re: [PATCH] iw: add kernel version checks for pending upstream kernel features

[Marcel Holtmann]

Hi Johannes,

> > if the numbers you are planning to use are stable, then please use the
> > above approach to just add the missing defines to it. Like adding a
> > compat.h file with
> > 
> > #ifndef NL80211_ATTR_SUPPORTED_IFTYPES
> > #define NL80211_ATTR_SUPPORTED_IFTYPES boo
> > #endif
> 
> Well, at that point I can also just copy nl80211.h from wireless-testing
> whenever I think the numbers will remain stable, no? And that doesn't
> work as-is anyway since nl80211.h uses an enum to define the numbers,
> not #defines.

it is possible to work around with just define the enum entries also as
defines. Netlink does this a lot. It is not pretty, but possible.

> I guess it's the lesser of all the evils. I'm not too fond of copying in
> the header file, it creates more work and doesn't make all that much
> sense, but all the other proposals take even more work, and as long as
> we don't change the numbers after putting things into wireless-testing
> we should be fine.

Simple speaking, yes :)

Regards

Marcel

Re: [PATCH] iw: add kernel version checks for pending upstream kernel features

[John W. Linville]

On Thu, Sep 18, 2008 at 05:41:28PM +0200, Johannes Berg wrote:
> On Thu, 2008-09-18 at 10:35 -0400, John W. Linville wrote:
> >  
> > +/* This funcionality first appears "officially" in 2.6.28... */
> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,28)
> 
> This bites all developers of the tool. I'd use #ifdef
> NL80211_ATTR_SUPPORTED_IFTYPES like Jouni did for a bunch of things but
> I forgot to add
> #define NL80211_ATTR_SUPPORTED_IFTYPES NL80211_ATTR_SUPPORTED_IFTYPES
> to nl80211.h

That seems a bit error prone, but probably manageable.  As for
the developers, we _could_ have a nicer check that also tested for
DEVELOPER_MODE or something.

> On the other hand, I'm with Pavel in that it sucks if you have a tool
> which your distro compiled against 2.6.27 and then suddenly you want to
> upgrade your kernel to 2.6.28 and the features don't work...

FWIW, I don't see that as any worse than having a tool that supports
functionality that your kernel can't support.  Besides, if the new
functionality is worth having, some user will be happy to remind the
iw maintainer to rebuild and push an update (at least in the land
of Fedora).

> I don't know. Tell me what to do. I don't really like shipping the
> header file either but it may be the lesser of two evils?

It was still early in the morning here on the left coast, so I was
forgetting that netlink-based API could be loosely couple w/ the
kernel, so it would be OK to have the tool know about options that
the kernel didn't support.  I think the best option is probably to
snapshot nl80211.h (or possible a sanitized version of it) inside
the iw source tree.

John
-- 
John W. Linville
linville@tuxdriver.com