linux-kernel - RE: [PATCH] llist: Put parentheses around parameters of llist_for_each_entry

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 26 Sep 2017 17:01:50 +0900
From:   박병철/선임연구원/SW Platform(연)AOT팀(byungchul.park@....com) 
        <byungchul.park@....com>
To:     "Huang, Ying" <ying.huang@...el.com>,
        Byungchul Park <byungchul.park@....com>
CC:     "peterz@...radead.org" <peterz@...radead.org>,
        "mingo@...nel.org" <mingo@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kernel-team@....com" <kernel-team@....com>
Subject: RE: [PATCH] llist: Put parentheses around parameters of
 llist_for_each_entry_safe()

> -----Original Message-----
> From: Huang, Ying [mailto:ying.huang@...el.com]
> Sent: Tuesday, September 26, 2017 4:02 PM
> To: Byungchul Park
> Cc: peterz@...radead.org; mingo@...nel.org; linux-kernel@...r.kernel.org;
> kernel-team@....com; ying.huang@...el.com
> Subject: Re: [PATCH] llist: Put parentheses around parameters of
> llist_for_each_entry_safe()
> 
> Hi, Byungchul,
> 
> Byungchul Park <byungchul.park@....com> writes:
> 
> > It would be somewhat safer to put parentheses around parameters of
> > a macro with parameters. Put it.
> >
> > Signed-off-by: Byungchul Park <byungchul.park@....com>
> > ---
> >  include/linux/llist.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/llist.h b/include/linux/llist.h
> > index 1957635..e280b297 100644
> > --- a/include/linux/llist.h
> > +++ b/include/linux/llist.h
> > @@ -183,10 +183,10 @@ static inline void init_llist_head(struct llist_head *list)
> >   * reverse the order by yourself before traversing.
> >   */
> >  #define llist_for_each_entry_safe(pos, n, node, member)
> 	       \
> > -	for (pos = llist_entry((node), typeof(*pos), member);		       \
> > +	for ((pos) = llist_entry((node), typeof(*(pos)), member);		       \
> >  	     member_address_is_nonnull(pos, member) &&
> 	       \
> > -	        (n = llist_entry(pos->member.next, typeof(*n), member), true); \
> > -	     pos = n)
> > +	        ((n) = llist_entry((pos)->member.next, typeof(*(n)), member), true);
> \
> > +	     (pos) = (n))
> >
> >  /**
> >   * llist_empty - tests whether a lock-less list is empty
> 
> The original code follows the style of list_for_each_entry_safe().  The

Hello Huang,

I don’t see what you say here exactly, but let me note that all llist macros
are safe except the llist_for_each_entry_safe().

> parameters "pos" and "n" must be variable.  Because list_xxx family
> functions work well so far, I think we needn't to change it too.

I see. I don't want to argue much wrt such a trivial thing but I think
it would be better to fix it since the fix is fairly simple and clear. 
However, it's ok if the fix introduces a bad thing at least.

> Best Regards,
> Huang, Ying