Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1322351 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/4] netfilter: ipv4: use preferred kernel types Lucas Tanure <tanure@linux.com> - 2016-01-30 14:20 +0100
    Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types Patrick McHardy <kaber@trash.net> - 2016-01-30 14:50 +0100
      Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types Lucas Tanure <tanure@linux.com> - 2016-01-30 15:10 +0100
        Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types Joe Perches <joe@perches.com> - 2016-01-30 18:30 +0100
        Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types Eric Dumazet <eric.dumazet@gmail.com> - 2016-01-30 19:00 +0100
          Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types Joe Perches <joe@perches.com> - 2016-01-30 19:30 +0100
            Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types Lucas Tanure <tanure@linux.com> - 2016-01-30 19:50 +0100
    RE: [PATCH 3/4] netfilter: ipv4: use preferred kernel types David Laight <David.Laight@ACULAB.COM> - 2016-02-01 17:50 +0100
      Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types David Miller <davem@davemloft.net> - 2016-02-01 20:50 +0100
        Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types Tom Herbert <tom@herbertland.com> - 2016-02-01 21:10 +0100

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

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

[toc] | [next] | [standalone]

#1322363

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(-)

[toc] | [prev] | [next] | [standalone]

#1322369

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!

[toc] | [prev] | [next] | [standalone]

#1322447

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'

[toc] | [prev] | [next] | [standalone]

#1322470

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.

[toc] | [prev] | [next] | [standalone]

#1322484

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.

[toc] | [prev] | [next] | [standalone]

#1322490

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.

[toc] | [prev] | [next] | [standalone]

#1323312

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

[toc] | [prev] | [next] | [standalone]

#1323436

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.

[toc] | [prev] | [next] | [standalone]

#1323446

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web