[PATCH 1/4] netfilter: ipv4: use linux/uaccess.h

[PATCH 1/4] netfilter: ipv4: use linux/uaccess.h

[Lucas Tanure]

Fix checkpatch warning
WARNING: Use #include <linux/uaccess.h> instead of <asm/uaccess.h>

Signed-off-by: Lucas Tanure <tanure@linux.com>
---
 net/ipv4/netfilter/ip_tables.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index b99affa..aa0e41e 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -20,7 +20,7 @@
 #include <linux/icmp.h>
 #include <net/ip.h>
 #include <net/compat.h>
-#include <asm/uaccess.h>
+#include <linux/uaccess.h>
 #include <linux/mutex.h>
 #include <linux/proc_fs.h>
 #include <linux/err.h>
-- 
2.7.0

[PATCH 2/4] netfilter: ipv4: EXPORT_SYMBOL should be shortly thereafter the exported function

[Lucas Tanure]

change made to resolve following checkpatch message:
  WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable

Signed-off-by: Lucas Tanure <tanure@linux.com>
---
 net/ipv4/netfilter/ip_tables.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index aa0e41e..79d4a04 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -442,6 +442,7 @@ ipt_do_table(struct sk_buff *skb,
 	else return verdict;
 #endif
 }
+EXPORT_SYMBOL(ipt_do_table);
 
 /* Figures out from what hook each rule can be called: returns 0 if
    there are loops.  Puts hook bitmask in comefrom. */
@@ -2098,6 +2099,7 @@ out_free:
 out:
 	return ERR_PTR(ret);
 }
+EXPORT_SYMBOL(ipt_register_table);
 
 void ipt_unregister_table(struct net *net, struct xt_table *table)
 {
@@ -2116,6 +2118,7 @@ void ipt_unregister_table(struct net *net, struct xt_table *table)
 		module_put(table_owner);
 	xt_free_table_info(private);
 }
+EXPORT_SYMBOL(ipt_unregister_table);
 
 /* Returns 1 if the type and code is matched by the range, 0 otherwise */
 static inline bool
@@ -2269,8 +2272,5 @@ static void __exit ip_tables_fini(void)
 	unregister_pernet_subsys(&ip_tables_net_ops);
 }
 
-EXPORT_SYMBOL(ipt_register_table);
-EXPORT_SYMBOL(ipt_unregister_table);
-EXPORT_SYMBOL(ipt_do_table);
 module_init(ip_tables_init);
 module_exit(ip_tables_fini);
-- 
2.7.0

[PATCH 3/4] netfilter: ipv4: use preferred kernel types

[Lucas Tanure]

As suggested by checkpatch.pl:
CHECK: Prefer kernel type 'uX' over 'uintX_t'

Signed-off-by: Lucas Tanure <tanure@linux.com>
---
 net/ipv4/netfilter/ip_tables.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 79d4a04..dcc61c0 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -1411,7 +1411,7 @@ compat_copy_entry_to_user(struct ipt_entry *e, void __user **dstptr,
 {
 	struct xt_entry_target *t;
 	struct compat_ipt_entry __user *ce;
-	u_int16_t target_offset, next_offset;
+	u16 target_offset, next_offset;
 	compat_uint_t origsize;
 	const struct xt_entry_match *ematch;
 	int ret = 0;
@@ -2122,8 +2122,7 @@ EXPORT_SYMBOL(ipt_unregister_table);
 
 /* Returns 1 if the type and code is matched by the range, 0 otherwise */
 static inline bool
-icmp_type_code_match(u_int8_t test_type, u_int8_t min_code, u_int8_t max_code,
-		     u_int8_t type, u_int8_t code,
+icmp_type_code_match(u8 test_type, u8 min_code, u8 max_code, u8 type, u8 code,
 		     bool invert)
 {
 	return ((test_type == 0xFF) ||
-- 
2.7.0

Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types

[Patrick McHardy]

On 30.01, Lucas Tanure wrote:
> As suggested by checkpatch.pl:
> CHECK: Prefer kernel type 'uX' over 'uintX_t'

You might have noticed we have literally hundreds of them spread over 100
files in the netfilter code. We'll gradually change them when the code is
touched anyways.

>  net/ipv4/netfilter/ip_tables.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types

[Lucas Tanure]

On Sat, Jan 30, 2016 at 11:45 AM, Patrick McHardy <kaber@trash.net> wrote:
> On 30.01, Lucas Tanure wrote:
>> As suggested by checkpatch.pl:
>> CHECK: Prefer kernel type 'uX' over 'uintX_t'
>
> You might have noticed we have literally hundreds of them spread over 100
> files in the netfilter code. We'll gradually change them when the code is
> touched anyways.
>
>>  net/ipv4/netfilter/ip_tables.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)

Yes, I checked that. But would be better to change that now?
Because:
- could take years to anyone to touch the code, as the code already
works very well
- be more standardized could facilitate reading the code
- It's a good way to encourage new people to contribute to the code

Thanks!

Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types

[Joe Perches]

On Sat, 2016-01-30 at 12:05 -0200, Lucas Tanure wrote:
> On Sat, Jan 30, 2016 at 11:45 AM, Patrick McHardy <kaber@trash.net> wrote:
> > On 30.01, Lucas Tanure wrote:
> > > As suggested by checkpatch.pl:
> > > CHECK: Prefer kernel type 'uX' over 'uintX_t'
> > 
> > You might have noticed we have literally hundreds of them spread over 100
> > files in the netfilter code. We'll gradually change them when the code is
> > touched anyways.
> > 
> > >  net/ipv4/netfilter/ip_tables.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> Yes, I checked that. But would be better to change that now?
> Because:
> - could take years to anyone to touch the code, as the code already
> works very well
> - be more standardized could facilitate reading the code
> - It's a good way to encourage new people to contribute to the code

The last one bullet point is what staging is for.

It might be better to do them all at once:

$ git grep --name-only "\bu_int" net/netfilter/ | \
  xargs perl -p -i -e 's/\bu_int(\d+)_t\b/u\1/g'

Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types

[Eric Dumazet]

On Sat, 2016-01-30 at 12:05 -0200, Lucas Tanure wrote:
> On Sat, Jan 30, 2016 at 11:45 AM, Patrick McHardy <kaber@trash.net> wrote:
> > On 30.01, Lucas Tanure wrote:
> >> As suggested by checkpatch.pl:
> >> CHECK: Prefer kernel type 'uX' over 'uintX_t'
> >
> > You might have noticed we have literally hundreds of them spread over 100
> > files in the netfilter code. We'll gradually change them when the code is
> > touched anyways.
> >
> >>  net/ipv4/netfilter/ip_tables.c | 5 ++---
> >>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> Yes, I checked that. But would be better to change that now?
> Because:
> - could take years to anyone to touch the code, as the code already
> works very well
> - be more standardized could facilitate reading the code
> - It's a good way to encourage new people to contribute to the code
> 
> Thanks!

These changes are a pain for people having to constantly backport fixes
into stable kernels, or rebase their patches before upstream
submissions.

Things like 'git cherry-pick' , 'git rebase' no longer work.
This is a huge pain, and manual editing to resolve conflicts often
add bugs.

Really, do you believe the 'uX' over 'uintX_t' stuff really matters for
people working on adding new features and fixing bugs ?

I am certain that if you had to work like us, you would quickly see the
utility of such changes is negative.

Sure, new submissions should be clean, but 'fixing' old code is not
worth it.

Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types

[Joe Perches]

On Sat, 2016-01-30 at 09:51 -0800, Eric Dumazet wrote:
> On Sat, 2016-01-30 at 12:05 -0200, Lucas Tanure wrote:
> > On Sat, Jan 30, 2016 at 11:45 AM, Patrick McHardy <kaber@trash.net> wrote:
> > > On 30.01, Lucas Tanure wrote:
> > > > As suggested by checkpatch.pl:
> > > > CHECK: Prefer kernel type 'uX' over 'uintX_t'
> > > 
> > > You might have noticed we have literally hundreds of them spread over 100
> > > files in the netfilter code. We'll gradually change them when the code is
> > > touched anyways.
> > > 
> > > >  net/ipv4/netfilter/ip_tables.c | 5 ++---
> > > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > Yes, I checked that. But would be better to change that now?
> > Because:
> > - could take years to anyone to touch the code, as the code already
> > works very well
> > - be more standardized could facilitate reading the code
> > - It's a good way to encourage new people to contribute to the code
> > 
> > Thanks!
> 
> These changes are a pain for people having to constantly backport fixes
> into stable kernels, or rebase their patches before upstream
> submissions.
> 
> Things like 'git cherry-pick' , 'git rebase' no longer work.
> This is a huge pain, and manual editing to resolve conflicts often
> add bugs.
> 
> Really, do you believe the 'uX' over 'uintX_t' stuff really matters for
> people working on adding new features and fixing bugs ?
> 
> I am certain that if you had to work like us, you would quickly see the
> utility of such changes is negative.
> 
> Sure, new submissions should be clean, but 'fixing' old code is not
> worth it.

That might depend on whether or not the linux kernel is
a "long-life project" and whether or no any old branch
of it is also important and sufficiently long-life.

The active life of a backport branch for the linux kernel
seems to be 3 or 4 years.  The linux kernel will likely
be useful for a few more decades beyond that.

Complex and long-life projects like the linux kernel
might benefit more in code complexity reduction patches
like these rather than code stasis for backward porting
ease.

In general, arguing for stasis leads to ossification,
slow decline.

Change for change's sake is poor, but changes to reduce
complexity, improve maintainability (for some measure of
it) and especially improve performance should be
welcomed where feasible.

Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types

[Lucas Tanure]

On Sat, Jan 30, 2016 at 4:26 PM, Joe Perches <joe@perches.com> wrote:
> On Sat, 2016-01-30 at 09:51 -0800, Eric Dumazet wrote:
>> On Sat, 2016-01-30 at 12:05 -0200, Lucas Tanure wrote:
>> > On Sat, Jan 30, 2016 at 11:45 AM, Patrick McHardy <kaber@trash.net> wrote:
>> > > On 30.01, Lucas Tanure wrote:
>> > > > As suggested by checkpatch.pl:
>> > > > CHECK: Prefer kernel type 'uX' over 'uintX_t'
>> > >
>> > > You might have noticed we have literally hundreds of them spread over 100
>> > > files in the netfilter code. We'll gradually change them when the code is
>> > > touched anyways.
>> > >
>> > > >  net/ipv4/netfilter/ip_tables.c | 5 ++---
>> > > >  1 file changed, 2 insertions(+), 3 deletions(-)
>> >
>> > Yes, I checked that. But would be better to change that now?
>> > Because:
>> > - could take years to anyone to touch the code, as the code already
>> > works very well
>> > - be more standardized could facilitate reading the code
>> > - It's a good way to encourage new people to contribute to the code
>> >
>> > Thanks!
>>
>> These changes are a pain for people having to constantly backport fixes
>> into stable kernels, or rebase their patches before upstream
>> submissions.
>>
>> Things like 'git cherry-pick' , 'git rebase' no longer work.
>> This is a huge pain, and manual editing to resolve conflicts often
>> add bugs.
>>
>> Really, do you believe the 'uX' over 'uintX_t' stuff really matters for
>> people working on adding new features and fixing bugs ?
>>
>> I am certain that if you had to work like us, you would quickly see the
>> utility of such changes is negative.
>>
>> Sure, new submissions should be clean, but 'fixing' old code is not
>> worth it.
>
> That might depend on whether or not the linux kernel is
> a "long-life project" and whether or no any old branch
> of it is also important and sufficiently long-life.
>
> The active life of a backport branch for the linux kernel
> seems to be 3 or 4 years.  The linux kernel will likely
> be useful for a few more decades beyond that.
>
> Complex and long-life projects like the linux kernel
> might benefit more in code complexity reduction patches
> like these rather than code stasis for backward porting
> ease.
>
> In general, arguing for stasis leads to ossification,
> slow decline.
>
> Change for change's sake is poor, but changes to reduce
> complexity, improve maintainability (for some measure of
> it) and especially improve performance should be
> welcomed where feasible.
>

My goal was to improve maintainability for the code, and with time,
contribute with meaningful code.
As you might have noticed I didn't fix every checkpatch.pl warning and error.
I just sent the ones that I thought would improve the maintainability.

And backport fixes will always be a pain, no matter what.

Thanks for all the comments.
Sorry for anything.

RE: [PATCH 3/4] netfilter: ipv4: use preferred kernel types

[David Laight]

From: Lucas Tanure
> Sent: 30 January 2016 13:18
> As suggested by checkpatch.pl:
> CHECK: Prefer kernel type 'uX' over 'uintX_t'

One might ask why?
The kernel types are older, but the uintX_t ones are now part
of the C standard.

Writing header files (eg for ioctl buffers) that have to be
parsed by both userspace and kernel is a PITA unless the uintX_t
forms are used - at which point you have inconsistent names
in the same file.

At some point someone might decide that the uintX_t types are
preferred (as NetBSD did a few years ago) - so these changes
would get reverted.

	David

Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types

[David Miller]

From: David Laight <David.Laight@ACULAB.COM>
Date: Mon, 1 Feb 2016 16:37:41 +0000

> From: Lucas Tanure
>> Sent: 30 January 2016 13:18
>> As suggested by checkpatch.pl:
>> CHECK: Prefer kernel type 'uX' over 'uintX_t'
> 
> One might ask why?

We have consistently done this, and consistency is enough of an
argument.

Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types

[Tom Herbert]

On Mon, Feb 1, 2016 at 11:41 AM, David Miller <davem@davemloft.net> wrote:
> From: David Laight <David.Laight@ACULAB.COM>
> Date: Mon, 1 Feb 2016 16:37:41 +0000
>
>> From: Lucas Tanure
>>> Sent: 30 January 2016 13:18
>>> As suggested by checkpatch.pl:
>>> CHECK: Prefer kernel type 'uX' over 'uintX_t'
>>
>> One might ask why?
>
> We have consistently done this, and consistency is enough of an
> argument.

Also, uintX_t is verbose and does not add anything to readability.

[PATCH 4/4] netfilter: ipv4: spaces preferred around operators

[Lucas Tanure]

Fix the checkpatch.pl issues:
CHECK: spaces preferred around that '&' (ctx:VxV)
CHECK: spaces preferred around that '<<' (ctx:VxV)
CHECK: spaces preferred around that '-' (ctx:VxV)
CHECK: spaces preferred around that '+' (ctx:VxV)

Signed-off-by: Lucas Tanure <tanure@linux.com>
---
 net/ipv4/netfilter/ip_tables.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index dcc61c0..6f29382 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -82,9 +82,9 @@ ip_packet_match(const struct iphdr *ip,
 
 #define FWINV(bool, invflg) ((bool) ^ !!(ipinfo->invflags & (invflg)))
 
-	if (FWINV((ip->saddr&ipinfo->smsk.s_addr) != ipinfo->src.s_addr,
+	if (FWINV((ip->saddr & ipinfo->smsk.s_addr) != ipinfo->src.s_addr,
 		  IPT_INV_SRCIP) ||
-	    FWINV((ip->daddr&ipinfo->dmsk.s_addr) != ipinfo->dst.s_addr,
+	    FWINV((ip->daddr & ipinfo->dmsk.s_addr) != ipinfo->dst.s_addr,
 		  IPT_INV_DSTIP)) {
 		dprintf("Source or dest mismatch.\n");
 
@@ -126,7 +126,7 @@ ip_packet_match(const struct iphdr *ip,
 
 	/* If we have a fragment rule but the packet is not a fragment
 	 * then we return zero */
-	if (FWINV((ipinfo->flags&IPT_F_FRAG) && !isfrag, IPT_INV_FRAG)) {
+	if (FWINV((ipinfo->flags & IPT_F_FRAG) && !isfrag, IPT_INV_FRAG)) {
 		dprintf("Fragment rule but not fragment.%s\n",
 			ipinfo->invflags & IPT_INV_FRAG ? " (INV)" : "");
 		return false;
@@ -496,7 +496,7 @@ mark_source_chains(const struct xt_table_info *newinfo,
 				/* Return: backtrack through the last
 				   big jump. */
 				do {
-					e->comefrom ^= (1<<NF_INET_NUMHOOKS);
+					e->comefrom ^= (1 << NF_INET_NUMHOOKS);
 #ifdef DEBUG_IP_FIREWALL_USER
 					if (e->comefrom
 					    & (1 << NF_INET_NUMHOOKS)) {
@@ -981,7 +981,7 @@ copy_entries_to_user(unsigned int total_size,
 					 + offsetof(struct xt_entry_match,
 						    u.user.name),
 					 m->u.kernel.match->name,
-					 strlen(m->u.kernel.match->name)+1)
+					 strlen(m->u.kernel.match->name) + 1)
 			    != 0) {
 				ret = -EFAULT;
 				goto free_counters;
@@ -993,7 +993,7 @@ copy_entries_to_user(unsigned int total_size,
 				 + offsetof(struct xt_entry_target,
 					    u.user.name),
 				 t->u.kernel.target->name,
-				 strlen(t->u.kernel.target->name)+1) != 0) {
+				 strlen(t->u.kernel.target->name) + 1) != 0) {
 			ret = -EFAULT;
 			goto free_counters;
 		}
@@ -1094,7 +1094,7 @@ static int get_info(struct net *net, void __user *user,
 	if (copy_from_user(name, user, sizeof(name)) != 0)
 		return -EFAULT;
 
-	name[XT_TABLE_MAXNAMELEN-1] = '\0';
+	name[XT_TABLE_MAXNAMELEN - 1] = '\0';
 #ifdef CONFIG_COMPAT
 	if (compat)
 		xt_compat_lock(AF_INET);
@@ -1270,7 +1270,7 @@ do_replace(struct net *net, const void __user *user, unsigned int len)
 	if (tmp.num_counters == 0)
 		return -EINVAL;
 
-	tmp.name[sizeof(tmp.name)-1] = 0;
+	tmp.name[sizeof(tmp.name) - 1] = 0;
 
 	newinfo = xt_alloc_table_info(tmp.size);
 	if (!newinfo)
@@ -1818,7 +1818,7 @@ compat_do_replace(struct net *net, void __user *user, unsigned int len)
 	if (tmp.num_counters == 0)
 		return -EINVAL;
 
-	tmp.name[sizeof(tmp.name)-1] = 0;
+	tmp.name[sizeof(tmp.name) - 1] = 0;
 
 	newinfo = xt_alloc_table_info(tmp.size);
 	if (!newinfo)
@@ -2041,7 +2041,7 @@ do_ipt_get_ctl(struct sock *sk, int cmd, void __user *user, int *len)
 			ret = -EFAULT;
 			break;
 		}
-		rev.name[sizeof(rev.name)-1] = 0;
+		rev.name[sizeof(rev.name) - 1] = 0;
 
 		if (cmd == IPT_SO_GET_REVISION_TARGET)
 			target = 1;
@@ -2155,7 +2155,7 @@ icmp_match(const struct sk_buff *skb, struct xt_action_param *par)
 				    icmpinfo->code[0],
 				    icmpinfo->code[1],
 				    ic->type, ic->code,
-				    !!(icmpinfo->invflags&IPT_ICMP_INV));
+				    !!(icmpinfo->invflags & IPT_ICMP_INV));
 }
 
 static int icmp_checkentry(const struct xt_mtchk_param *par)
@@ -2188,13 +2188,13 @@ static struct xt_target ipt_builtin_tg[] __read_mostly = {
 static struct nf_sockopt_ops ipt_sockopts = {
 	.pf		= PF_INET,
 	.set_optmin	= IPT_BASE_CTL,
-	.set_optmax	= IPT_SO_SET_MAX+1,
+	.set_optmax	= IPT_SO_SET_MAX + 1,
 	.set		= do_ipt_set_ctl,
 #ifdef CONFIG_COMPAT
 	.compat_set	= compat_do_ipt_set_ctl,
 #endif
 	.get_optmin	= IPT_BASE_CTL,
-	.get_optmax	= IPT_SO_GET_MAX+1,
+	.get_optmax	= IPT_SO_GET_MAX + 1,
 	.get		= do_ipt_get_ctl,
 #ifdef CONFIG_COMPAT
 	.compat_get	= compat_do_ipt_get_ctl,
-- 
2.7.0