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

Groups > linux.kernel > #1323861 > unrolled thread

[PATCH] kernel/fork.c: use sizeof() instead of sizeof

Back to article view | Back to linux.kernel

Contents

  [PATCH] kernel/fork.c: use sizeof() instead of sizeof Wei Tang <tangwei@cmss.chinamobile.com> - 2016-02-02 09:00 +0100
    Re: [PATCH] kernel/fork.c: use sizeof() instead of sizeof Ingo Molnar <mingo@kernel.org> - 2016-02-02 10:20 +0100
      Re: [PATCH] treewide: Use 'sizeof(x)' instead of 'sizeof x' Joe Perches <joe@perches.com> - 2016-02-02 15:00 +0100
      Re: [PATCH] kernel/fork.c: use sizeof() instead of sizeof Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-02 16:50 +0100
        Re: [PATCH] kernel/fork.c: use sizeof() instead of sizeof Joe Perches <joe@perches.com> - 2016-02-02 18:10 +0100
          Re: [PATCH] kernel/fork.c: use sizeof() instead of sizeof Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-02 18:20 +0100
            Re: [PATCH] kernel/fork.c: use sizeof() instead of sizeof Joe Perches <joe@perches.com> - 2016-02-02 19:00 +0100
        Re: [PATCH] kernel/fork.c: use sizeof() instead of sizeof Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-02 18:10 +0100
          Re: [PATCH] kernel/fork.c: use sizeof() instead of sizeof Kevin Easton <kevin@guarana.org> - 2016-02-03 05:00 +0100
            Re: [PATCH] kernel/fork.c: use sizeof() instead of sizeof Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-03 05:10 +0100

#1323861 — [PATCH] kernel/fork.c: use sizeof() instead of sizeof

This patch fixes the checkpatch.pl warning to fork.c:

WARNING: sizeof sig->rlim should be sizeof(sig->rlim)

Signed-off-by: Wei Tang <tangwei@cmss.chinamobile.com>
---
 kernel/fork.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 2e391c7..30e04d2 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1143,7 +1143,7 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
 	sig->real_timer.function = it_real_fn;
 
 	task_lock(current->group_leader);
-	memcpy(sig->rlim, current->signal->rlim, sizeof sig->rlim);
+	memcpy(sig->rlim, current->signal->rlim, sizeof(sig->rlim));
 	task_unlock(current->group_leader);
 
 	posix_cpu_timers_init_group(sig);
-- 
1.9.1

[toc] | [next] | [standalone]

#1323901

* Wei Tang <tangwei@cmss.chinamobile.com> wrote:

> This patch fixes the checkpatch.pl warning to fork.c:
> 
> WARNING: sizeof sig->rlim should be sizeof(sig->rlim)
> 
> Signed-off-by: Wei Tang <tangwei@cmss.chinamobile.com>
> ---
>  kernel/fork.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 2e391c7..30e04d2 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1143,7 +1143,7 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
>  	sig->real_timer.function = it_real_fn;
>  
>  	task_lock(current->group_leader);
> -	memcpy(sig->rlim, current->signal->rlim, sizeof sig->rlim);
> +	memcpy(sig->rlim, current->signal->rlim, sizeof(sig->rlim));
>  	task_unlock(current->group_leader);
>  
>  	posix_cpu_timers_init_group(sig);

So there's over 1,000 such occurances in the kernel and we do not need this 
drip-drip churn...

If anyone feels strongly about accepting such patches, then the right solution is 
to create a Coccinelle semantic patch to run over the whole kernel and get over 
with the churn once and for all.

Thanks,

	Ingo

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

#1324106 — Re: [PATCH] treewide: Use 'sizeof(x)' instead of 'sizeof x'

On Tue, 2016-02-02 at 12:28 +0100, Ingo Molnar wrote:
> * Ingo Molnar <mingo@kernel.org> wrote:
> 
> > If anyone feels strongly about accepting such patches, then the right solution 
> > is to create a Coccinelle semantic patch to run over the whole kernel and get 
> > over with the churn once and for all.
> 
> So applying a semantic patch like this to all .c files:
> 
>   @@ expression E; @@
>   -sizeof E
>   +sizeof(E)
>   @@ expression E2; @@
>   -sizeof((E2))
>   +sizeof(E2)
> 
> Produces the single patch below for the whole kernel - instead of generating a 
> churn of 1,000+ patches ...

If this isn't a joke, a nicer way to submit this is
by subsystem and not as a single huge patch.

This allows subsystem maintainers to reduce patch
contention as this patch already doesn't apply
to -next.

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

#1324215

On Tue, Feb 02, 2016 at 10:11:18AM +0100, Ingo Molnar wrote:
> 
> * Wei Tang <tangwei@cmss.chinamobile.com> wrote:
> 
> > This patch fixes the checkpatch.pl warning to fork.c:
> > 
> > WARNING: sizeof sig->rlim should be sizeof(sig->rlim)
> > 
> > Signed-off-by: Wei Tang <tangwei@cmss.chinamobile.com>
> > ---
> >  kernel/fork.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 2e391c7..30e04d2 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -1143,7 +1143,7 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
> >  	sig->real_timer.function = it_real_fn;
> >  
> >  	task_lock(current->group_leader);
> > -	memcpy(sig->rlim, current->signal->rlim, sizeof sig->rlim);
> > +	memcpy(sig->rlim, current->signal->rlim, sizeof(sig->rlim));
> >  	task_unlock(current->group_leader);
> >  
> >  	posix_cpu_timers_init_group(sig);
> 
> So there's over 1,000 such occurances in the kernel and we do not need this 
> drip-drip churn...
> 
> If anyone feels strongly about accepting such patches, then the right solution is 
> to create a Coccinelle semantic patch to run over the whole kernel and get over 
> with the churn once and for all.

That, or a single patch taking that piece of idiocy out of checkpatch.pl...

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

#1324287

On Tue, 2016-02-02 at 15:45 +0000, Al Viro wrote:
> On Tue, Feb 02, 2016 at 10:11:18AM +0100, Ingo Molnar wrote:
> > * Wei Tang <tangwei@cmss.chinamobile.com> wrote:
[]
> > > WARNING: sizeof sig->rlim should be sizeof(sig->rlim)
[]
> > If anyone feels strongly about accepting such patches, then the right solution is 
> > to create a Coccinelle semantic patch to run over the whole kernel and get over 
> > with the churn once and for all.
> 
> That, or a single patch taking that piece of idiocy out of checkpatch.pl...

https://lkml.org/lkml/2012/7/11/103

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

#1324296

On Tue, Feb 02, 2016 at 09:02:16AM -0800, Joe Perches wrote:
> On Tue, 2016-02-02 at 15:45 +0000, Al Viro wrote:
> > On Tue, Feb 02, 2016 at 10:11:18AM +0100, Ingo Molnar wrote:
> > > * Wei Tang <tangwei@cmss.chinamobile.com> wrote:
> []
> > > > WARNING: sizeof sig->rlim should be sizeof(sig->rlim)
> []
> > > If anyone feels strongly about accepting such patches, then the right solution is 
> > > to create a Coccinelle semantic patch to run over the whole kernel and get over 
> > > with the churn once and for all.
> > 
> > That, or a single patch taking that piece of idiocy out of checkpatch.pl...
> 
> https://lkml.org/lkml/2012/7/11/103

Umm...  Matter of taste, really - and I don't quite agree about "should think
of sizeof() as a function, not as some ass-backwards special case C parsing
rule that is subtle as hell".  I've seen enough folks getting confused about
treatment of arrays; confusion between sizeof uses and function calls often
contributed to that.

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

#1324341

On Tue, 2016-02-02 at 17:11 +0000, Al Viro wrote:
> On Tue, Feb 02, 2016 at 09:02:16AM -0800, Joe Perches wrote:
> > On Tue, 2016-02-02 at 15:45 +0000, Al Viro wrote:
> > > On Tue, Feb 02, 2016 at 10:11:18AM +0100, Ingo Molnar wrote:
> > > > * Wei Tang <tangwei@cmss.chinamobile.com> wrote:
> > []
> > > > > WARNING: sizeof sig->rlim should be sizeof(sig->rlim)
> > []
> > > > If anyone feels strongly about accepting such patches, then the right solution is 
> > > > to create a Coccinelle semantic patch to run over the whole kernel and get over 
> > > > with the churn once and for all.
> > > 
> > > That, or a single patch taking that piece of idiocy out of checkpatch.pl...
> > 
> > https://lkml.org/lkml/2012/7/11/103
> 
> Umm...  Matter of taste, really

On that I can agree.  Idiocy rather less so.

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

#1324289

On Tue, Feb 02, 2016 at 03:56:01PM +0000, Aleksa Sarai wrote:

> I haven't looked at the order of operations for sizeof, but I imagine
> there's cases where it might bind in a different way than is expected. Are
> you sure there'd be no negative downside to removing the check (the whole
> point is to ensure no new code has stuff like that).

I won't comment on the whole point (or lack thereof) of checkpatch.pl, but
the only subtlety with sizeof is that sizeof ( type-name ) <something> is
*never* interpreted as sizeof of something cast to type-name.  IOW, its
priority is higher than that of typecasts.  If <something> starts with {,
it's a sizeof of compound literal, otherwise it's an unary expression
"sizeof ( type-name )" followed by <something>, which might or might not
yield a valid expression (e.g.
	sizeof(int)1
won't parse, while
	sizeof(int)-1
will be treated as (sizeof(int)) - 1).

Potential headache is along the lines of
#define A (int)-1
sizeof A
but that's more of "use enough parentheses in body of a macro to avoid nasty
surprises" - same as e.g.
#define A 2 + 2
A * A
yielding 8 (2 + 2 * 2 + 2) rather than expected 16 ((2 + 2) * (2 + 2))

FWIW, the actual rules are
	unary-expression: postfix-expression |
			  ++ unary-expression |
			  -- unary-expression |
			  - cast-expression |
			  + cast-expression |
			  ! cast-expression |
			  ~ cast-expression |
			  * cast-expression |
			  & cast-expression |
			  sizeof unary-expression |
			  sizeof ( type-name )
	cast-expression: unary-expression |
			 ( type-name ) cast-expression
Note that while e.g.
	++ ++ n
is allowed by grammar, it runs afoul of the constraint for ++ argument, which
must be a modifiable lvalue.  None of the operators above yield that, so
the rules for ++ and -- might as well have been ++ postfix-expression and
-- postfix-expression resp.

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

#1324891

On Tue, Feb 02, 2016 at 05:04:06PM +0000, Al Viro wrote:
 
> FWIW, the actual rules are
> 	unary-expression: postfix-expression |
> 			  ++ unary-expression |
> 			  -- unary-expression |
> 			  - cast-expression |
> 			  + cast-expression |
> 			  ! cast-expression |
> 			  ~ cast-expression |
> 			  * cast-expression |
> 			  & cast-expression |
> 			  sizeof unary-expression |
> 			  sizeof ( type-name )
> 	cast-expression: unary-expression |
> 			 ( type-name ) cast-expression
> Note that while e.g.
> 	++ ++ n
> is allowed by grammar, it runs afoul of the constraint for ++ argument, which
> must be a modifiable lvalue.  None of the operators above yield that, so
> the rules for ++ and -- might as well have been ++ postfix-expression and
> -- postfix-expression resp.

Unless I'm mistaken, * cast-expression yields an lvalue.

    - Kevin

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

#1324894

On Tue, Feb 02, 2016 at 10:46:07PM -0500, Kevin Easton wrote:
> On Tue, Feb 02, 2016 at 05:04:06PM +0000, Al Viro wrote:

> > the rules for ++ and -- might as well have been ++ postfix-expression and
> > -- postfix-expression resp.
> 
> Unless I'm mistaken, * cast-expression yields an lvalue.

D'oh...  Obviously correct (and just as obviously, ++*p is valid and quite
common).  Shouldn't have posted while half-asleep ;-/  Self-LART applied...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web