[PATCH] ipv4: devconf: start IPV4_DEVCONF_* from 0

[PATCH] ipv4: devconf: start IPV4_DEVCONF_* from 0

[Lucian Adrian Grijincu]

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

The IPV4_DEVCONF_* enums are never exposed to the userspace and it
would make code simpler to remove all the useless (-1) adjustments.

-----

Thomas Graf added an interface that exposes the enums (counting from 1)
for libnl which seems to be the only user of the interface:

commit 9f0f7272ac9506f4c8c05cc597b7e376b0b9f3e4
Author: Thomas Graf <tgraf@infradead.org>
Date:   Tue Nov 16 04:32:48 2010 +0000

    ipv4: AF_INET link address family

The development branch of libnl[0] redefines the enums in userspace
counting from 1 (to mimic the 2.6.37 kernel).

The stable[1] version of libnl does not define or use these enums.

This patch adjusts the number received from userspace to not break the
ABI, but given these circumstances, could we consider the 2.6.37 ABI
unstable and change the only user to start counting from 0?

[0] http://git.kernel.org/?p=libs/netlink/libnl.git
[1] http://git.kernel.org/?p=libs/netlink/libnl-stable.git

-----

    And the moral of the story is that we had better regard — after
    all those centuries! — zero as a most natural number.

    http://www.cs.utexas.edu/users/EWD/transcriptions/EWD08xx/EWD831.html

CC: Edsger Wybe Dijkstra <dijkstra@cs.utexas.edu>
CC: Thomas Graf <tgraf@infradead.org>
Signed-off-by: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>
---
 include/linux/inetdevice.h |    9 +++------
 net/ipv4/devinet.c         |   44 ++++++++++++++++++++++++++------------------
 2 files changed, 29 insertions(+), 24 deletions(-)

[-- Attachment #2: 0003-ipv4-devconf-start-IPV4_DEVCONF_-from-0.patch --]
[-- Type: text/x-patch, Size: 5017 bytes --]

diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index ae8fdc5..e4d93b2 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -13,7 +13,7 @@
 
 enum
 {
-	IPV4_DEVCONF_FORWARDING=1,
+	IPV4_DEVCONF_FORWARDING,
 	IPV4_DEVCONF_MC_FORWARDING,
 	IPV4_DEVCONF_PROXY_ARP,
 	IPV4_DEVCONF_ACCEPT_REDIRECTS,
@@ -38,10 +38,9 @@ enum
 	IPV4_DEVCONF_ACCEPT_LOCAL,
 	IPV4_DEVCONF_SRC_VMARK,
 	IPV4_DEVCONF_PROXY_ARP_PVLAN,
-	__IPV4_DEVCONF_MAX
+	IPV4_DEVCONF_MAX
 };
 
-#define IPV4_DEVCONF_MAX (__IPV4_DEVCONF_MAX - 1)
 
 struct ipv4_devconf {
 	void	*sysctl;
@@ -72,20 +71,18 @@ struct in_device {
 	struct rcu_head		rcu_head;
 };
 
-#define IPV4_DEVCONF(cnf, attr) ((cnf).data[IPV4_DEVCONF_ ## attr - 1])
+#define IPV4_DEVCONF(cnf, attr) ((cnf).data[IPV4_DEVCONF_ ## attr])
 #define IPV4_DEVCONF_ALL(net, attr) \
 	IPV4_DEVCONF((*(net)->ipv4.devconf_all), attr)
 
 static inline int ipv4_devconf_get(struct in_device *in_dev, int index)
 {
-	index--;
 	return in_dev->cnf.data[index];
 }
 
 static inline void ipv4_devconf_set(struct in_device *in_dev, int index,
 				    int val)
 {
-	index--;
 	set_bit(index, in_dev->cnf.state);
 	in_dev->cnf.data[index] = val;
 }
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 748cb5b..181d558 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -65,20 +65,20 @@
 
 static struct ipv4_devconf ipv4_devconf = {
 	.data = {
-		[IPV4_DEVCONF_ACCEPT_REDIRECTS - 1] = 1,
-		[IPV4_DEVCONF_SEND_REDIRECTS - 1] = 1,
-		[IPV4_DEVCONF_SECURE_REDIRECTS - 1] = 1,
-		[IPV4_DEVCONF_SHARED_MEDIA - 1] = 1,
+		[IPV4_DEVCONF_ACCEPT_REDIRECTS] = 1,
+		[IPV4_DEVCONF_SEND_REDIRECTS]   = 1,
+		[IPV4_DEVCONF_SECURE_REDIRECTS] = 1,
+		[IPV4_DEVCONF_SHARED_MEDIA]     = 1,
 	},
 };
 
 static struct ipv4_devconf ipv4_devconf_dflt = {
 	.data = {
-		[IPV4_DEVCONF_ACCEPT_REDIRECTS - 1] = 1,
-		[IPV4_DEVCONF_SEND_REDIRECTS - 1] = 1,
-		[IPV4_DEVCONF_SECURE_REDIRECTS - 1] = 1,
-		[IPV4_DEVCONF_SHARED_MEDIA - 1] = 1,
-		[IPV4_DEVCONF_ACCEPT_SOURCE_ROUTE - 1] = 1,
+		[IPV4_DEVCONF_ACCEPT_REDIRECTS]    = 1,
+		[IPV4_DEVCONF_SEND_REDIRECTS]      = 1,
+		[IPV4_DEVCONF_SECURE_REDIRECTS]    = 1,
+		[IPV4_DEVCONF_SHARED_MEDIA]        = 1,
+		[IPV4_DEVCONF_ACCEPT_SOURCE_ROUTE] = 1,
 	},
 };
 
@@ -1304,12 +1304,15 @@ static int inet_validate_link_af(const struct net_device *dev,
 
 	if (tb[IFLA_INET_CONF]) {
 		nla_for_each_nested(a, tb[IFLA_INET_CONF], rem) {
-			int cfgid = nla_type(a);
+			/* Userspace indexes these ids starting from 1.
+			 * This was the way the kernel indexed elements too,
+			 * but now it counts from 0 */
+			int cfgid = nla_type(a) - 1;
 
 			if (nla_len(a) < 4)
 				return -EINVAL;
 
-			if (cfgid <= 0 || cfgid > IPV4_DEVCONF_MAX)
+			if (cfgid < 0 || cfgid >= IPV4_DEVCONF_MAX)
 				return -EINVAL;
 		}
 	}
@@ -1330,8 +1333,13 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla)
 		BUG();
 
 	if (tb[IFLA_INET_CONF]) {
-		nla_for_each_nested(a, tb[IFLA_INET_CONF], rem)
-			ipv4_devconf_set(in_dev, nla_type(a), nla_get_u32(a));
+		nla_for_each_nested(a, tb[IFLA_INET_CONF], rem) {
+			/* Userspace indexes these ids starting from 1.
+			 * This was the way the kernel indexed elements too,
+			 * but now it counts from 0 */
+			int cfgid = nla_type(a) - 1;
+			ipv4_devconf_set(in_dev, cfgid, nla_get_u32(a));
+		}
 	}
 
 	return 0;
@@ -1449,7 +1457,7 @@ static int ipv4_doint_and_flush(ctl_table *ctl, int write,
 	{ \
 		.procname	= name, \
 		.data		= ipv4_devconf.data + \
-				  IPV4_DEVCONF_ ## attr - 1, \
+				  IPV4_DEVCONF_ ## attr, \
 		.maxlen		= sizeof(int), \
 		.mode		= mval, \
 		.proc_handler	= proc, \
@@ -1470,7 +1478,7 @@ static int ipv4_doint_and_flush(ctl_table *ctl, int write,
 
 static struct devinet_sysctl_table {
 	struct ctl_table_header *sysctl_header;
-	struct ctl_table devinet_vars[__IPV4_DEVCONF_MAX];
+	struct ctl_table devinet_vars[IPV4_DEVCONF_MAX + 1];
 	char *dev_name;
 } devinet_sysctl = {
 	.devinet_vars = {
@@ -1505,6 +1513,7 @@ static struct devinet_sysctl_table {
 					      "force_igmp_version"),
 		DEVINET_SYSCTL_FLUSHING_ENTRY(PROMOTE_SECONDARIES,
 					      "promote_secondaries"),
+		{ }
 	},
 };
 
@@ -1590,8 +1599,7 @@ static void devinet_sysctl_unregister(struct in_device *idev)
 static struct ctl_table ctl_forward_entry[] = {
 	{
 		.procname	= "ip_forward",
-		.data		= &ipv4_devconf.data[
-					IPV4_DEVCONF_FORWARDING - 1],
+		.data		= &ipv4_devconf.data[IPV4_DEVCONF_FORWARDING],
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= devinet_sysctl_forward,
@@ -1635,7 +1643,7 @@ static __net_init int devinet_init_net(struct net *net)
 		if (tbl == NULL)
 			goto err_alloc_ctl;
 
-		tbl[0].data = &all->data[IPV4_DEVCONF_FORWARDING - 1];
+		tbl[0].data = &all->data[IPV4_DEVCONF_FORWARDING];
 		tbl[0].extra1 = all;
 		tbl[0].extra2 = net;
 #endif

Re: [PATCH] ipv4: devconf: start IPV4_DEVCONF_* from 0

[David Miller]

From: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>
Date: Wed, 12 Jan 2011 12:19:10 +0200

> The IPV4_DEVCONF_* enums are never exposed to the userspace and it
> would make code simpler to remove all the useless (-1) adjustments.

Starting values like this at "1" is usually done on purpose.

It allows "0" to be illegal or mean "none", and thus easily trapping
cases where the value fails to be initialized properly.  In this way
the illegal sentinel "0" doesn't take up any space either.

Re: [PATCH] ipv4: devconf: start IPV4_DEVCONF_* from 0

[Lucian Adrian Grijincu]

On Thu, Jan 13, 2011 at 4:47 AM, David Miller <davem@davemloft.net> wrote:
> From: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>
> Date: Wed, 12 Jan 2011 12:19:10 +0200
>
>> The IPV4_DEVCONF_* enums are never exposed to the userspace and it
>> would make code simpler to remove all the useless (-1) adjustments.
>
> Starting values like this at "1" is usually done on purpose.
>
> It allows "0" to be illegal or mean "none", and thus easily trapping
> cases where the value fails to be initialized properly.  In this way
> the illegal sentinel "0" doesn't take up any space either.


Just that no one checks for zero as invalid anywhere.

We pass the enum names everywhere as parameters. And wherever we need
to use those values we must make sure to subtract 1 every time.

And some things work ok, but it's not entirely obvious why.

For example:
	struct ctl_table devinet_vars[__IPV4_DEVCONF_MAX] (...) = {
                DEVINET_SYSCTL_COMPLEX_ENTRY(FORWARDING, "forwarding",
                                             devinet_sysctl_forward),

This works ok because the DEVINET_SYSCTL_* macros subtract 1 from each
array index.

Because the size of the array is __IPV4_DEVCONF_MAX (without
subtracting 1), there's an extra element at the end and because this
is a global definition it gets initialized with zeros just as required
by register_net_sysctl_table: the last element's procname must be zero
to indicate end-of-array.

Yes it works, but there does not seem to be a good reason why to
complicate things like this (again the sentinel nature of zero is not
used in any place here).


-- 
 .
..: Lucian

Re: [PATCH] ipv4: devconf: start IPV4_DEVCONF_* from 0

[Thomas Graf]

On Thu, Jan 13, 2011 at 09:50:14AM +0200, Lucian Adrian Grijincu wrote:
> Yes it works, but there does not seem to be a good reason why to
> complicate things like this (again the sentinel nature of zero is not
> used in any place here).

The reason I didn't change anything was the same as Dave's reply, I
thought it must have been done on purpose. It probably was but I can't
spot any reason now either.

Also, IPv6 is doing just fine with using '0' as its first devconf id.

I have no objects to changing this at all but we don't gain much either.

Re: [PATCH] ipv4: devconf: start IPV4_DEVCONF_* from 0

[Lucian Adrian Grijincu]

On Thu, Jan 13, 2011 at 12:02 PM, Thomas Graf <tgraf@infradead.org> wrote:
> On Thu, Jan 13, 2011 at 09:50:14AM +0200, Lucian Adrian Grijincu wrote:
>> Yes it works, but there does not seem to be a good reason why to
>> complicate things like this (again the sentinel nature of zero is not
>> used in any place here).
>
> I have no objects to changing this at all but we don't gain much either.


Should I post a new patch in which IFLA_INET_CONF cfgid values start
from 0 also or must the ABI for libnl be kept as-is (counting from 1)?


-- 
 .
..: Lucian

Re: [PATCH] ipv4: devconf: start IPV4_DEVCONF_* from 0

[Alexey Kuznetsov]

Hello!

On Thu, Jan 13, 2011 at 05:02:24AM -0500, Thomas Graf wrote:
> The reason I didn't change anything was the same as Dave's reply, I
> thought it must have been done on purpose.

The days, when this was written, 0 was interpreted as end of array by sysctl core.
BTW, -1=CTL_ANY also used to be special, that's why NET_PROTO_CONF_ALL=-2 rather than -1.

So, yes, it was done on purpose. And, yes, this purpose is not a legal purpose now.

Alexey

Re: [PATCH] ipv4: devconf: start IPV4_DEVCONF_* from 0

[David Miller]

From: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>
Date: Thu, 13 Jan 2011 14:23:41 +0200

> On Thu, Jan 13, 2011 at 12:02 PM, Thomas Graf <tgraf@infradead.org> wrote:
>> On Thu, Jan 13, 2011 at 09:50:14AM +0200, Lucian Adrian Grijincu wrote:
>>> Yes it works, but there does not seem to be a good reason why to
>>> complicate things like this (again the sentinel nature of zero is not
>>> used in any place here).
>>
>> I have no objects to changing this at all but we don't gain much either.
> 
> 
> Should I post a new patch in which IFLA_INET_CONF cfgid values start
> from 0 also or must the ABI for libnl be kept as-is (counting from 1)?

I think the conclusion is that we won't apply your change, sorry.

There is almost zero upside, and one known downside in that we have
to deal with this user visible breakage somehow.