[PATCH] rculist: fix borked __list_for_each_rcu() macro

[PATCH] rculist: fix borked __list_for_each_rcu() macro

[Mariusz Kozlowski]

This restores parentheses blance.

Signed-off-by: Mariusz Kozlowski <mk@lab.zgora.pl>
---
 include/linux/rculist.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index f31ef61..70d3ba5 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -244,7 +244,7 @@ static inline void list_splice_init_rcu(struct list_head *list,
 #define __list_for_each_rcu(pos, head) \
 	for (pos = rcu_dereference_raw(list_next_rcu(head)); \
 		pos != (head); \
-		pos = rcu_dereference_raw(list_next_rcu((pos)))
+		pos = rcu_dereference_raw(list_next_rcu(pos)))
 
 /**
  * list_for_each_entry_rcu	-	iterate over rcu list of given type
-- 
1.7.0.4

Re: [PATCH] rculist: fix borked __list_for_each_rcu() macro

[Paul E. McKenney]

On Wed, Dec 15, 2010 at 11:11:12PM +0100, Mariusz Kozlowski wrote:
> This restores parentheses blance.

Good catch, queued!!!

This does not actually appear to be in use anywhere in the kernel any
more, so I queued this for 2.6.38 rather than in the 2.6.37 urgent queue.
So, just out of curiosity, how did you find this one?

Hmmm...  Maybe we should just delete __list_for_each_rcu.  ;-)

							Thanx, Paul

> Signed-off-by: Mariusz Kozlowski <mk@lab.zgora.pl>
> ---
>  include/linux/rculist.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index f31ef61..70d3ba5 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -244,7 +244,7 @@ static inline void list_splice_init_rcu(struct list_head *list,
>  #define __list_for_each_rcu(pos, head) \
>  	for (pos = rcu_dereference_raw(list_next_rcu(head)); \
>  		pos != (head); \
> -		pos = rcu_dereference_raw(list_next_rcu((pos)))
> +		pos = rcu_dereference_raw(list_next_rcu(pos)))
> 
>  /**
>   * list_for_each_entry_rcu	-	iterate over rcu list of given type
> -- 
> 1.7.0.4
> 

Re: [PATCH] rculist: fix borked __list_for_each_rcu() macro

[Mariusz Kozlowski]

On Wed, Dec 15, 2010 at 03:20:05PM -0800, Paul E. McKenney wrote:
> On Wed, Dec 15, 2010 at 11:11:12PM +0100, Mariusz Kozlowski wrote:
> > This restores parentheses blance.
> 
> Good catch, queued!!!
> 
> This does not actually appear to be in use anywhere in the kernel any
> more, so I queued this for 2.6.38 rather than in the 2.6.37 urgent queue.
> So, just out of curiosity, how did you find this one?

Some years ago I wrote a dumb script that walks trees of () and {}.
It catches unbalanced trees. It's dumb enough to fail with #ifdef etc,
but most of the time it does its job. It reaches unreachable code
and unused one too.

> Hmmm...  Maybe we should just delete __list_for_each_rcu.  ;-)
> 
> 							Thanx, Paul
> 
> > Signed-off-by: Mariusz Kozlowski <mk@lab.zgora.pl>
> > ---
> >  include/linux/rculist.h |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > index f31ef61..70d3ba5 100644
> > --- a/include/linux/rculist.h
> > +++ b/include/linux/rculist.h
> > @@ -244,7 +244,7 @@ static inline void list_splice_init_rcu(struct list_head *list,
> >  #define __list_for_each_rcu(pos, head) \
> >  	for (pos = rcu_dereference_raw(list_next_rcu(head)); \
> >  		pos != (head); \
> > -		pos = rcu_dereference_raw(list_next_rcu((pos)))
> > +		pos = rcu_dereference_raw(list_next_rcu(pos)))
> > 
> >  /**
> >   * list_for_each_entry_rcu	-	iterate over rcu list of given type
> > -- 
> > 1.7.0.4
> > 

-- 
Mariusz Kozlowski

Re: [PATCH] rculist: fix borked __list_for_each_rcu() macro

[Américo Wang]

On Thu, Dec 16, 2010 at 07:02:36AM +0100, Mariusz Kozlowski wrote:
>On Wed, Dec 15, 2010 at 03:20:05PM -0800, Paul E. McKenney wrote:
>> On Wed, Dec 15, 2010 at 11:11:12PM +0100, Mariusz Kozlowski wrote:
>> > This restores parentheses blance.
>> 
>> Good catch, queued!!!
>> 
>> This does not actually appear to be in use anywhere in the kernel any
>> more, so I queued this for 2.6.38 rather than in the 2.6.37 urgent queue.
>> So, just out of curiosity, how did you find this one?
>
>Some years ago I wrote a dumb script that walks trees of () and {}.
>It catches unbalanced trees. It's dumb enough to fail with #ifdef etc,
>but most of the time it does its job. It reaches unreachable code
>and unused one too.
>

gcc will complain about this, however, in this case, it is used.

Re: [PATCH] rculist: fix borked __list_for_each_rcu() macro

[Paul E. McKenney]

On Thu, Dec 16, 2010 at 03:38:40PM +0800, Américo Wang wrote:
> On Thu, Dec 16, 2010 at 07:02:36AM +0100, Mariusz Kozlowski wrote:
> >On Wed, Dec 15, 2010 at 03:20:05PM -0800, Paul E. McKenney wrote:
> >> On Wed, Dec 15, 2010 at 11:11:12PM +0100, Mariusz Kozlowski wrote:
> >> > This restores parentheses blance.
> >> 
> >> Good catch, queued!!!
> >> 
> >> This does not actually appear to be in use anywhere in the kernel any
> >> more, so I queued this for 2.6.38 rather than in the 2.6.37 urgent queue.
> >> So, just out of curiosity, how did you find this one?
> >
> >Some years ago I wrote a dumb script that walks trees of () and {}.
> >It catches unbalanced trees. It's dumb enough to fail with #ifdef etc,
> >but most of the time it does its job. It reaches unreachable code
> >and unused one too.
> 
> gcc will complain about this, however, in this case, it is used.

Hello, Américo!

I did a "git grep -l __list_for_each_rcu" and its output was only:

	include/linux/rculist.h:#define __list_for_each_rcu(pos, head) \

This was in Linus's tree.  And gcc certainly would have failed if
this macro had been used in any recent build.

It really was used some time back, but it appears to me that those
uses no longer exist.

Or are you saying that you have a patch on its way in that needs
this macro?  If so, please point me at it.

							Thanx, Paul

Re: [PATCH] rculist: fix borked __list_for_each_rcu() macro

[Paul E. McKenney]

On Thu, Dec 16, 2010 at 07:02:36AM +0100, Mariusz Kozlowski wrote:
> On Wed, Dec 15, 2010 at 03:20:05PM -0800, Paul E. McKenney wrote:
> > On Wed, Dec 15, 2010 at 11:11:12PM +0100, Mariusz Kozlowski wrote:
> > > This restores parentheses blance.
> > 
> > Good catch, queued!!!
> > 
> > This does not actually appear to be in use anywhere in the kernel any
> > more, so I queued this for 2.6.38 rather than in the 2.6.37 urgent queue.
> > So, just out of curiosity, how did you find this one?
> 
> Some years ago I wrote a dumb script that walks trees of () and {}.
> It catches unbalanced trees. It's dumb enough to fail with #ifdef etc,
> but most of the time it does its job. It reaches unreachable code
> and unused one too.

Very cool, and thank you!

							Thanx, Paul

> > Hmmm...  Maybe we should just delete __list_for_each_rcu.  ;-)
> > 
> > 							Thanx, Paul
> > 
> > > Signed-off-by: Mariusz Kozlowski <mk@lab.zgora.pl>
> > > ---
> > >  include/linux/rculist.h |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > > index f31ef61..70d3ba5 100644
> > > --- a/include/linux/rculist.h
> > > +++ b/include/linux/rculist.h
> > > @@ -244,7 +244,7 @@ static inline void list_splice_init_rcu(struct list_head *list,
> > >  #define __list_for_each_rcu(pos, head) \
> > >  	for (pos = rcu_dereference_raw(list_next_rcu(head)); \
> > >  		pos != (head); \
> > > -		pos = rcu_dereference_raw(list_next_rcu((pos)))
> > > +		pos = rcu_dereference_raw(list_next_rcu(pos)))
> > > 
> > >  /**
> > >   * list_for_each_entry_rcu	-	iterate over rcu list of given type
> > > -- 
> > > 1.7.0.4
> > > 
> 
> -- 
> Mariusz Kozlowski

Re: [PATCH] rculist: fix borked __list_for_each_rcu() macro

[Américo Wang]

On Thu, Dec 16, 2010 at 07:50:54AM -0800, Paul E. McKenney wrote:
>On Thu, Dec 16, 2010 at 03:38:40PM +0800, Américo Wang wrote:
>> On Thu, Dec 16, 2010 at 07:02:36AM +0100, Mariusz Kozlowski wrote:
>> >On Wed, Dec 15, 2010 at 03:20:05PM -0800, Paul E. McKenney wrote:
>> >> On Wed, Dec 15, 2010 at 11:11:12PM +0100, Mariusz Kozlowski wrote:
>> >> > This restores parentheses blance.
>> >> 
>> >> Good catch, queued!!!
>> >> 
>> >> This does not actually appear to be in use anywhere in the kernel any
>> >> more, so I queued this for 2.6.38 rather than in the 2.6.37 urgent queue.
>> >> So, just out of curiosity, how did you find this one?
>> >
>> >Some years ago I wrote a dumb script that walks trees of () and {}.
>> >It catches unbalanced trees. It's dumb enough to fail with #ifdef etc,
>> >but most of the time it does its job. It reaches unreachable code
>> >and unused one too.
>> 
>> gcc will complain about this, however, in this case, it is used.
>
>Hello, Américo!
>
>I did a "git grep -l __list_for_each_rcu" and its output was only:
>
>	include/linux/rculist.h:#define __list_for_each_rcu(pos, head) \
>
>This was in Linus's tree.  And gcc certainly would have failed if
>this macro had been used in any recent build.
>

Yeah, my bad, actually I meant to say "unused"... :-(
Sorry for confusing!

My point is that gcc should do this basic lexical check, no need
to invent another tool. :)

Thanks.

Re: [PATCH] rculist: fix borked __list_for_each_rcu() macro

[Paul E. McKenney]

On Fri, Dec 17, 2010 at 06:10:39PM +0800, Américo Wang wrote:
> On Thu, Dec 16, 2010 at 07:50:54AM -0800, Paul E. McKenney wrote:
> >On Thu, Dec 16, 2010 at 03:38:40PM +0800, Américo Wang wrote:
> >> On Thu, Dec 16, 2010 at 07:02:36AM +0100, Mariusz Kozlowski wrote:
> >> >On Wed, Dec 15, 2010 at 03:20:05PM -0800, Paul E. McKenney wrote:
> >> >> On Wed, Dec 15, 2010 at 11:11:12PM +0100, Mariusz Kozlowski wrote:
> >> >> > This restores parentheses blance.
> >> >> 
> >> >> Good catch, queued!!!
> >> >> 
> >> >> This does not actually appear to be in use anywhere in the kernel any
> >> >> more, so I queued this for 2.6.38 rather than in the 2.6.37 urgent queue.
> >> >> So, just out of curiosity, how did you find this one?
> >> >
> >> >Some years ago I wrote a dumb script that walks trees of () and {}.
> >> >It catches unbalanced trees. It's dumb enough to fail with #ifdef etc,
> >> >but most of the time it does its job. It reaches unreachable code
> >> >and unused one too.
> >> 
> >> gcc will complain about this, however, in this case, it is used.
> >
> >Hello, Américo!
> >
> >I did a "git grep -l __list_for_each_rcu" and its output was only:
> >
> >	include/linux/rculist.h:#define __list_for_each_rcu(pos, head) \
> >
> >This was in Linus's tree.  And gcc certainly would have failed if
> >this macro had been used in any recent build.
> 
> Yeah, my bad, actually I meant to say "unused"... :-(
> Sorry for confusing!

No problem!

> My point is that gcc should do this basic lexical check, no need
> to invent another tool. :)

As an off-by-default warning, this could make a lot of sense, especially
for projects like the Linux kernel that are relatively disciplined in
their use of cpp macros.  Though I am not sure that the recent macros
in the "perf" code would pass such a check.  ;-)

							Thanx, Paul